Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

unix, windows: add uv_loop_alive() function #813

Merged
merged 1 commit into from
Dec 19, 2013
Merged

unix, windows: add uv_loop_alive() function #813

merged 1 commit into from
Dec 19, 2013

Conversation

sam-github
Copy link

Useful to know when the the event loop is empty, this can't be done with
uv_run() without possibly blocking, or running some events (which might
empty the event loop as a side-effect).

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Sam Roberts

Please see CONTRIBUTING.md for more information

@saghul
Copy link
Contributor

saghul commented May 29, 2013

You can achieve the same with uv_run by using UV_RUN_NOWAIT mode. It returns 0 when the loop is finished and != 0 when there is still work to be done and uv_run should be called again.

if (!uv_run(loop, UV_RUN_NOWAIT)) {
    // loop is not alive
    ...
}

@sam-github
Copy link
Author

Using NOWAIT was my first attempt, it looked promising, but won't work.

Problem is that it tells you that the loop is no longer alive after call, but not whether a handler had to be run to get down to zero.

Look at the linked pull request. If you try to rewrite using RUN_NOWAIT, you get this:

while(1) {
uv_run(ALL);
emit('keepalive');
// if nothing was scheduled, exit
r = uv_run(ONCE);
if(!r) {
// hm, maybe one thing was scheduled, then run, and now we should re-emit the keep alive
// or... maybe nothing was scheduled, and we should exit
// which is it? no way to know

@saghul
Copy link
Contributor

saghul commented May 29, 2013

Oh, I see.

* This function checks whether the reference count, the number of active
* handles or requests left in the event loop, is non-zero.
*/
UV_EXTERN int uv_loop_alive(uv_loop_t* loop);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const uv_loop_t*?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires uv__loop_alive() to take const, as well. Is that OK?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's fine. It should have been const in the first place, really.

@bnoordhuis
Copy link
Contributor

LGTM but a test would be nice.

@sam-github
Copy link
Author

I was hoping to avoid the test (because its a renamed version of an existing function), but I will write one. Speaking of which, I could just change the name of the existing private uv__loop_alive() and make its argument const, rather than having uv_loop_alive() wrap the private. Would you prefer that?

@bnoordhuis
Copy link
Contributor

I was hoping to avoid the test (because its a renamed version of an existing function), but I will write one.

Rule of thumb is that all API additions should be covered by at least one test.

Speaking of which, I could just change the name of the existing private uv__loop_alive() and make its argument const, rather than having uv_loop_alive() wrap the private. Would you prefer that?

No. That makes it possible for outsiders to (inadvertently or otherwise) overload the uv_loop_alive symbol, plus it means each call goes through the PLT. It probably won't have a critical performance impact but it means I have to remember to set LD_BIND_NOW when I profile something.

(EDIT: Libuv is not very consistent in that regard; I tend to clean up such call sites when I encounter them.)

@sam-github
Copy link
Author

/to @bnoordhuis, has test that both handles and requests count as alive

@saghul
Copy link
Contributor

saghul commented Sep 23, 2013

Looks like this was forgotten. Can you rebase on top of master and force push? You shouldn't have any problem, but since we use autotools now you need to modify Makefile.am to add the test file, build.mk no longer exists.

Thanks!

Useful to know when the the event loop is empty, this can't be done with
uv_run() without possibly blocking, or running some events (which might
empty the event loop as a side-effect).
@saghul saghul merged commit ed36b85 into joyent:master Dec 19, 2013
@saghul
Copy link
Contributor

saghul commented Dec 19, 2013

Thanks @sam-github! ✨

@sam-github sam-github deleted the add_is_alive branch December 30, 2014 22:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants