Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

lep: Implementation of uv_callback #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

lep: Implementation of uv_callback #9

wants to merge 3 commits into from

Conversation

kroggen
Copy link

@kroggen kroggen commented Dec 12, 2016

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.

@bnoordhuis
Copy link
Member

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.

@kroggen
Copy link
Author

kroggen commented Dec 12, 2016

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.

@saghul
Copy link
Member

saghul commented Dec 12, 2016

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.

@kroggen
Copy link
Author

kroggen commented Dec 12, 2016

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.

@saghul
Copy link
Member

saghul commented Dec 12, 2016

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.

@kroggen
Copy link
Author

kroggen commented Dec 12, 2016

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!

@saghul
Copy link
Member

saghul commented Dec 13, 2016

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

@kroggen
Copy link
Author

kroggen commented Dec 13, 2016

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

@saghul
Copy link
Member

saghul commented Dec 13, 2016

Yes.

@kroggen
Copy link
Author

kroggen commented Apr 30, 2017

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants