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

Adding doc describing "async context" in terms of "continuations" #197

Closed

Conversation

mike-kaufman
Copy link
Contributor

@mike-kaufman mike-kaufman commented May 24, 2018

See here for a rendered view of the markdown.

This is a refinement of the work we've been doing in http://github.com/mike-kaufman/async-context-definition/. I would really appreciate any feedback people have on the terminology, concepts, correctness, or the flow of the document - my intent here is this is easily explainable to a broad section of JS programmers.

@mike-kaufman
Copy link
Contributor Author

//cc @nodejs/diagnostics, @mrkmarron, @ofrobots, @kjin

kjin

This comment was marked as off-topic.

@mike-kaufman
Copy link
Contributor Author

@kjin - thanks for comments, all makes sense. I'm going off the grid until next wed - will incorporate your feedback then.

ofrobots

This comment was marked as off-topic.

@littledan
Copy link

cc @domenic

@mcollina
Copy link
Member

mcollina commented Jun 2, 2018

Very good work.

In https://github.com/mike-kaufman/diagnostics/blob/11206a47094846bc0b1027ab82c7bd0fa4f04d87/async-context/README.md#continuation-local-storage I would use a private symbol to store the cls.

I think the example there of walking the "linking context" is very expensive. Moreover, I'm not necessarily convinced it works in the case of await. Consider the following:

async function f2 () {
  await something()
  // consider this point B
}

async function f1 () {
  await f2()

  // consider this point A
} 

The problem with walking the context during get() (as in the example in the document), is that whatever we add in point B will not be included in point A. I believe the users would expect the two continuations to be linked, but they are not.

This is because of how async await is specified (I'm not the best person to explain why, cc @bmeurer), and the promise f2() and the result of await are parallel continuations.

I do not think it is possible to handle CLS without a init-like hook to manually propagate the context instead of walking the linked contexts.

@vdeturckheim
Copy link
Member

Great stuff.

Can we define what is what I call a "continuation loss" meaning for some reason ContinuationInvocation.GetCurrent() ends up returning something wrong?

@mike-kaufman
Copy link
Contributor Author

Can we define ... a "continuation loss" meaning for some reason ContinuationInvocation.GetCurrent() ends up returning something wrong?

@vdeturckheim - can you provide some examples of where/why this would happen. Ideally, this is pathological behavior. i.e., we provide clear & crisp definitions, and the implementation returns "the right thing" based on these definitions. This is the "Async Call Graph".

Now, it's possible that different applications want to traverse the graph in different ways, but I wouldn't call this a "continuation loss".

Flarna

This comment was marked as off-topic.

Flarna

This comment was marked as off-topic.

@vdeturckheim
Copy link
Member

@mike-kaufman I agree this is a case that should not happen.
However, due to implementation details, it is not always possible to find a parent to a node of the graph (I had the issue with an hand-crafted promise library recently).

Flarna

This comment was marked as off-topic.

Flarna

This comment was marked as off-topic.

@mike-kaufman
Copy link
Contributor Author

@mcollina - thanks for the feedback. Please keep comments like this coming!

Sorry for length of post below. Happy to get on phone to clarify anything.

  1. RE "very expensive", I'm not following this, can you clarify? My take here is operations like I've described are equivalent to walking a list, and these are O(n) where n is the length of the "path-to-the-root" from the current node. I'd like to keep the "cost analysis" observations concrete - either runtime/memory big-O complexity of the algorithms being proposed, or grounded in real repeatable measurements. Please LMK if I'm missing something.

  2. RE the async/await example, I think it is helpful here to not conflate the structure of the graph with the algorithms over top of it. That is, I'd like to arrive at a clear set of definitions & rules, and we can apply those in a rigorous way, and the result is a data structure that accurately represents the "async call graph". Once we agree on these rules & the graph structure, then we can break higher-level problems down into "traversal" problems - i.e., does traversing a specific set of edges give us desired behavior? I think your comment is really a traversal problem in two parts, namely:
    i. what are user expectations of "CLS" in async-await examples?
    ii. how do we traverse the graph to achieve user-expected resutls?

I'll look into the implementation of generators and async/await in more detail to see check my assumptions here. For now, I'll break your example down into it's functionally equivalent promise-based implementation, since I think that makes the async boundaries more clear:

function f2 () {
  return somethingThatReturnsAPromise()
    .then(function f3() { // line 3
       // consider this point B
   });
}

function f1 () {
  return f2()
      .then(function f4() {  // line 10
         // consider this point A
  });
} 

Given definitions above, we have the following stack at the time that somethingThatReturnsAPromise() is called:

----------------------------------                ----------- 
| somethingThatReturnsAPromise() |                          |
----------------------------------                          \/                   
|    function f2()               |                  Continuation Invocation ci1
----------------------------------                          /\    
|    function f1()               |                          |
----------------------------------                          |
|    ...host code...             |                          |
======================================                      |
|   Root Continuation A()            |                      |
======================================            -----------

And we have the following stack at the time then is called on "line 3":

----------------------------------                ----------- 
|    promise.then() // line 3    |                          |
----------------------------------                          \/                   
|    function f2()               |                  Continuation Invocation ci1
----------------------------------                          /\    
|    function f1()               |                          |
----------------------------------                          |
|    ...host code...             |                          |
======================================                      |
|   Root Continuation A()            |                      |
======================================            -----------

And we have the following stack at the time then is called on "line 10":

----------------------------------                ----------- 
|    promise.then() // line 10   |                          |
----------------------------------                          \/
|    function f1()               |                  Continuation Invocation ci1
----------------------------------                          /\    
|    ...host code...             |                          |
======================================                      |
|   Root Continuation A()            |                      |
======================================            -----------

Note that all of the above are executing within the same Continuation Invocation, namely ci1. Now, when the continuations f3 and f4 are invoked, they each have their own continuation invocation instance - let's call these ci-f3 and ci-f4 respectively. The (simplified) Async Call Graph looks like this:

            ------- 
            | ci1 |
            -------
               /\
               |
   ---------------------------------
   |                               |
   |                               |
 "link edge"                   "link edge"
   |                               |
---------                       --------- 
| ci-f3 |<----"ready-edge"------| ci-f4 |
---------                       ---------

Now, to address your questions:

  1. Is it reasonable for a user to expect a CLS value stored in ci-f3 to be accessible in ci-f4? I don't claim to know the answer here, but I would argue that these are peer branches of the async call tree, thus such a value would not be "visible". Would love to hear other opinions/arguments here.

  2. If you wanted to make a value stored in ci-f3 accessible in ci-f4, can you do that? I claim the answer here is "yes", namely by traversing cif-f4's "ready-edge" on lookup.

I've been thinking a bit about how dynamically prescribe traversal paths through the graph for specific functionality. (e.g., how do we choose the "correct path" through the graph to do CLS"?) I think the answer here is to allow dynamic labels over the edges such that a "CLS iterator" can choose the path of edges labeled "CLS" as it traverses. This needs to be communicated in more detail, but I think it nicely accomodates the expected behavior you suggest.

@Flarna
Copy link
Member

Flarna commented Jun 4, 2018

I think besides linking continuations we have to think also about combining them - and decide which link context is the right one (or both?).
Think about following:

async function foo() {
  const p1 = doFoo();
  const p2 = doBar();
  return Promise.all([p1, p2]);
}
await foo();

Here we have two resolvers for the returned promise. In this sample the link context of p1 and p2 are the same but it should be not that hard to create a sample where there are different.

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

@gireeshpunathil
Copy link
Member

can this be landed? I skimmed through this an d looks like lot of useful information, but not sure how updated it is, given its age. @mhdawson @Qard - can you pls have a look and suggest?

@mhdawson
Copy link
Member

@gireeshpunathil I think we might as well land it and then we can update if needed.

@Qard
Copy link
Member

Qard commented Jul 31, 2021

Maybe merge and add a note at the top that the doc needs editing?

@RafaelGSS
Copy link
Member

Maybe merge and add a note at the top that the doc needs editing?

+1

gireeshpunathil pushed a commit that referenced this pull request Aug 14, 2021
Add a doc on async_context  in terms of continuations

PR-URL: #197
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@gireeshpunathil
Copy link
Member

landed in bdcfe0e

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

Successfully merging this pull request may close these issues.