src: add uv_loop watcher_queue update apis#1921
Conversation
|
@cjihrig any updates on this? |
cjihrig
left a comment
There was a problem hiding this comment.
I didn't add a test for the initial iteration of this API addition, but I can go ahead and add one if need be!
Yes, tests would be preferable.
Is Electron currently floating any similar patches, or is this fixing an existing bug?
|
@cjihrig we're currently floating this commit. i'll add a test soon! |
|
Sounds good, thanks. Do any other collaborators want to weigh in? |
cjihrig
left a comment
There was a problem hiding this comment.
If no other collaborator objects, this LGTM with tests.
|
Code LGTM but I was wondering if it would make sense integrating this into the tracing API proposal |
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
|
The Windows side to this is missing. Is this intentional? Also, this needs some docs explaining what the callback actually does and the platforms where it's supported. If this is only needed on Unix, how does Electron deal with this on Windows? |
|
bump @cjihrig the test should be in place now! i'll resolve conflicts asap once it looks good from the CI side :) |
|
@codebytere please see #1921 (comment) |
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
@saghul @codebytere |
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
|
@codebytere It got a lgtm and no nacks so I'd say yes, it's good to go after a rebase. |
|
Do we want to decide on anything regarding @santigimeno's #1921 (comment)? |
|
@cjihrig @bnoordhuis should be set to go, let me know if I should address anything else! |
|
@cjihrig Do you still expect that other PR to land someday? I wasn't a big fan last time I looked. |
|
I'm fine with landing this independent of #1815. I just didn't want Santi's comment to slip past. Also, sorry to @codebytere for the delay. |
|
EDIT: Nevermind, I see in an old comment this was discussed. |
Even so, wouldn't we want to provide the same API on Windows too? |
|
@bnoordhuis i think that should be everything; if you think this is good to retarget to |
|
I think targeting v1.x should be fine now. |
saghul
left a comment
There was a problem hiding this comment.
I don't think this can land in v1.x due to the ABI breakeage, plus I still want to see it implemented on Windows. The fact that Electron achieves the same goal in a different way is not relevant here, we strive to provide cross-platform consistency, so Windows should get this callback to function as well.
|
@bnoordhuis repointed and removed the |
|
@saghul mind taking a last look at this one? |
saghul
left a comment
There was a problem hiding this comment.
Left some comments, PTAL. Sorry, but I have to insist: Windows is still missing and I'm -1 to landing without it.
| unsigned int stop_flag; | ||
| void* reserved[4]; | ||
| uv_loop_cb watcher_queue_updated_cb; | ||
| void* reserved[3]; |
There was a problem hiding this comment.
Since this targets master now, please leave 4 reserved fields. We can break the ABI here.
| }; | ||
| #undef XX | ||
|
|
||
| typedef void (*uv_loop_cb)(uv_loop_t*); |
There was a problem hiding this comment.
can we give this a better name it reads like "loop callback" but we might introduce more in the future.
|
|
||
| .. c:function:: uv_loop_cb uv_loop_get_watcher_queue_changed_callback(uv_loop_t* loop) | ||
|
|
||
| Returns `loop->watcher_queue_updated_cb`. |
There was a problem hiding this comment.
Please explain what the function does, not how it's implemented.
| .. c:function:: void uv_loop_set_watcher_queue_changed_callback(uv_loop_t* loop, | ||
| uv_loop_cb cb) | ||
|
|
||
| Sets `loop->watcher_queue_updated_cb` to `cb`. |
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This commit should be removed when libuv/libuv#1921 is merged
|
@codebytere mentioned she plans on picking this up again so I'll go ahead and reopen this. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This PR adds to
uv_loop_ta callback fieldon_queue_watcher_updatedwhich is called whenever the loop's watcher queue changes. It also adds a public getter and setter for this field.Electron's Node Integration works by listening to Node's backend file descriptor in a separate thread; when an event is ready the backend file descriptor will trigger a new event for it, and the main thread will then iterate the libuv loop. For certain operations (ex. adding a timeout task) the backend file descriptor isn't informed, & as a result the main thread doesn't know it needs to iterate the libuv loop so the timeout task will never execute until something else trigger a new event. This PR mitigates that problem.
I didn't add a test for the initial iteration of this API addition, but I can go ahead and add one if need be!