leps icon indicating copy to clipboard operation
leps copied to clipboard

lep: Implementation of uv_callback

Open kroggen opened this issue 9 years ago • 10 comments

I don't know if this is the best place to post this proposal but my time is short, so I am sending it as a lep without asking.

If it does not fit, just ignore it.

kroggen avatar Dec 12 '16 04:12 kroggen

I don't think your proposal is bad but it seems to be building a higher-level API around uv_async_t. That's something we try to stay away from in libuv but it would be fine for a libuv-contrib or libuv-extras project. Libuv itself focuses on the building blocks.

bnoordhuis avatar Dec 12 '16 13:12 bnoordhuis

But it does not. It replaces uv_async.

Indeed what it does is rename uv_async with uv_callback and add more features to it. So we don't need to rewrite all the stuff from scratch.

As I am not able to implement all this by myself I supplied part of the code, but where there is uv_async_send it must be replaced by the code of uv_async_send.

The same for uv_async_t. This is just a first draft.

kroggen avatar Dec 12 '16 14:12 kroggen

Thanks for the writeup! I wish you had talked to us first though, specially since I proposed uv_callback a while ago, but it got stalled.

At the moment I'm with @bnoordhuis: uv_callback can be built atop atop uv_async + a thread safe queue. It looks like a good fit for libuv-extras.

As I am not able to implement all this by myself I supplied part of the code, but where there is uv_async_send it must be replaced by the code of uv_async_send.

The expectations from LEP writers is that they follow up with an implementation, once it has been approved from the README):

The author of a LEP is expected to actually pursue the proposal with code.

saghul avatar Dec 12 '16 15:12 saghul

Unhappily it cannot be implemented on top of uv_async because the code that calls the callback needs to read from the call queue, so we need at least to change the uv_async.

And if we will change it then there is no need to keep it. We can supersede it with uv_callback.

OK, I can try to continue the work. But I will need some tips, at least for the uv_async event internals.

I can start just changing the uv_async and running the tests. After it works, then we can go forward and rename all the stuff. If you approve.

kroggen avatar Dec 12 '16 16:12 kroggen

Unhappily it cannot be implemented on top of uv_async because the code that calls the callback needs to read from the call queue, so we need at least to change the uv_async.

I don't think so. You can use an async handle and its callback drain the queue. Then use an auxiliary idle handle for draining the queue again, in case more requests where sent while the queue was being drained.

I can start just changing the uv_async and running the tests. After it works, then we can go forward and rename all the stuff. If you approve.

Rename to what? I suggest you start by trying to implement it on top of uv_async, a QUEUE and a lock, then let's see what's missing! But as I said, I think it's possible. I implemented a very similar pattern though that was in Python with pyuv.

saghul avatar Dec 12 '16 17:12 saghul

I don't think so. You can use an async handle and its callback drain the queue. Then use an auxiliary idle handle for draining the queue again, in case more requests where sent while the queue was being drained.

Hmmm. You're right.

Thank you for your help!

kroggen avatar Dec 12 '16 19:12 kroggen

No problem @kroggen. I'll land the LEP as rejected later today, so we have on the record.

saghul avatar Dec 13 '16 08:12 saghul

But I have a question: do you plan to release v2 with the current uv_async_send?

kroggen avatar Dec 13 '16 15:12 kroggen

Yes.

saghul avatar Dec 13 '16 16:12 saghul

Hi guys!

I implemented this stuff (with a few modifications).

It is available here: uv_callback

It can be added in the libuv-extras if you think it fits the requirements.

If you have any suggestion for modification, please send me.

kroggen avatar Apr 30 '17 21:04 kroggen