[GitHub] [skywalking-nodejs] tom-pytel edited a comment on pull request #27: use invalid to check asyncState

[GitBox <gi...@apache.org>]

tom-pytel edited a comment on pull request #27:
URL: https://github.com/apache/skywalking-nodejs/pull/27#issuecomment-783284349


   > As indicated in the docs, it applies for the entire remainder of the current tick, which isn't necessarily the span you think it is. 
   
   I understand what you mean by this and I did test to verify how it behaves (event handlers and all). I take this into account and specifically code around it by doing things like cloning the context object in child tasks, including a mutable array of spans in order to not modify the parent. Also explicitly removing the current span from the list of spans if the span code is about to relinquish control to be regained in a new async task and re-adding to the list of spans there in order that chronologically subsequent siblings in the same task do not become children (does that explanation make sense?). It seems to work.
   
   > In cases like event batching it can cause all sorts of havoc because, as shown in the docs, it will leak from one event emission into the next if they are triggered within the same tick. There's no _logical_ boundary around `store.enterWith(...)` only a tick transition. 
   
   The logical boundary in this case is somewhat convoluted because of the open-ended control flow with `span.start()` and `span.stop()` which can be called anywhere, so I take care of enforcing the boundary. It is not as strict as `run()`, but I don't need that, just something that works like Python's `contextvars.ContextVar`.
   
   > The primary risk here is that even if it may work correctly _now_ you are relying on unspecified behaviour that may not remain true forever. 
   
   This is a risk, but I do not have much choice here. Like I said, `run()` is not really an option because of the open-ended control flow. If the behavior of `enterWith()` changes in the future I will either have to adapt to whatever the new situation is, or will have to fall back to what I am currently doing for Node 10 which is to emulate a `ContextVar`-ish `AsyncLocalStorage` using `asyc_hooks.createHook()` - which is obviously not ideal for performance reasons.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org