Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracking issue: passing custom async_id's to node::AsyncWrap::AsyncWrap #14209

Closed
trevnorris opened this issue Jul 12, 2017 · 6 comments
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@trevnorris
Copy link
Contributor

Tracking issue for difficulties using the API in #14208.

Here's a bullet list of issues, and walk through to explain the logic behind each conclusion:

Timers
The exposed APIs of active()/enroll()/unenroll() creates the following issues:

  1. Arbitrary objects can be passed to all three of those. Meaning constructors that have already had an async_id assigned would have a conflict.

    1. Possible solution: Piggy back on the preexisting async_id and simply not assign a new one, and skip running init().
      Conclusion: Not possible because an object that is meant to be assigned an async_id hasn't had it assigned before being passed to active(). Thus assigning one, and firing init() with a type === 'Timeout'.
    2. Possible solution: timers has it's own async_id_symbol separate from the one exported by process.binding('async_wrap') and have the object treated as a completely separate asynchronous resource.
      Conclusion: Most plausible option, but still fails in a few edge cases as will be discussed.
  2. Option [1.2] has the edge case of possibly not being able to acquire the correct trigger_id. For example:

    const { async_id_symbol } = process.binding('async_wrap');
    const s = new net.Socket();
    console.log(s[async_id_symbol] === -1)
    s.setTimeout(10, () => {});

    Though this case is simple, and the reason for the new API. What happens is the async_id is assigned in net.Socket() and that async_id is passed to TCPWrap.

    1. issue: Because init() won't fire until TCPWrap is constructed (which in this case isn't actually until .listen() for net.Server()) users will see async_ids in their graphs that haven't run init() yet.
    2. issue: More complex cases break down quickly. For example http.OutgoingMessage() isn't directly attached to the object that has the reference to the TCPWrap instance, but it still has its own setTimeout() method. Making it nearly impossible to pre-assign the async_id and then safely pass it down the object chain to reach TCPWrap.
  3. Usage of enroll() is completely optional, since any object can be passed directly to active(), but node internals uses both HandleWrap::Close() and/or unenroll() to close the resource.
    Conclusion: Calling destroy() for on the timer's async_id_symbol, an active() object needs to check the following things:

    1. Timeout#close() will emitDestroy() if timer._called === false and if the timer hasn't been unenroll()'d.
    2. After the _onTimeout() callback has run, regardless of whether the callback threw or not. If the error is handled then it'll be called later in the event loop. If not then the process is exiting and it didn't matter anyway.
      Note: timer._called is set to true before the callback runs, so calling timer.close() during the _onTimeout() callback that value is safe to check in Timeout#close().
    3. unenroll() will emitDestroy() if the timer hasn't been closed, and if timer._called === false.
      Note: Part of the problem here is that while you don't need to pass pass a Timeout() instance to active() or enroll() (though you can...) it is possible to pass a Timeout() instance to unenroll(). Which is basically the same as passing it to clearTimeout().
      Note: This means assigning the async_id_symbol (thus calling emitInit()) should happen in the call to active, and enroll() should simply be seen as a helper function to add properties to the object. But unenroll() can call emitDestroy(). This makes for a somewhat unintuitive API.

Note: This isn't everything, but is enough to get started. Will update soon with the remainder of the known edge cases.

cc @addaleax @AndreasMadsen @refack

@refack refack self-assigned this Jul 12, 2017
@mscdex mscdex added async_wrap timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jul 13, 2017
@AndreasMadsen
Copy link
Member

/cc @Fishrock123

@AndreasMadsen AndreasMadsen added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 14, 2017
@refack
Copy link
Contributor

refack commented Jul 15, 2017

To whomever wants to help us tackle this faster, I'm available on IRC and a few IM platforms.

@Jeyanthinath
Copy link
Contributor

Jeyanthinath commented Jul 26, 2017

I will give it a try @refack

@refack can you give your im uid, other than irc

@AkshayIyer12
Copy link
Contributor

Can I work on the issue?

@uttampawar
Copy link
Contributor

@trevnorris There is no activity since last year. Do you still need someone to work on this issue?

@AndreasMadsen
Copy link
Member

It has been implemented

@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

No branches or pull requests

7 participants