Feature request: API mechanism to await RequestContext death #411

Activity

1. jasnell commented on Mar 2, 2023

   

   jasnell

   on Mar 2, 2023

   Collaborator

   More actions

   One approach to handling this could be register a waitUntil promise that is waiting on all the other waitUntil promises to settle.

   For instance,

   const p1 = promiseToWaitOn();
   const p2 = promiseToWaitOn();
   
   async function assertSettled(...promises) {
     const results = await Promise.allSettled(promises);
     // verify results
   }
   
   ctx.waitUntil(p1);
   ctx.waitUntil(p2);
   ctx.waitUntil(assertSettled(p1, p2));
   

   Of course, this falls under the "alternate waitUntil magic" category but it should be workable.

   Do you have ideas on what the API mechanism you're envisioning would look like? Node.js has a notion of a final on('exit', ...) event that we use for these kinds of things. It is emitted exactly once immediately before the process exits.

   In Node.js' test suite, we have a utility called common.mustCall(...) that will throw an error if a wrapped function is not called during the process. There is a CallTracker class on the assert module that implements similar behavior (it's still experimental and hasn't received much love but it exists). Something similar could be adopted here. The key challenge, of course, is that rejected wait until promises do not cause failures and just end up stuck in logs somewhere. Having more of an idea of how you'd expect the mechanism to work would be helpful (also, would you expect the mechanism to be available on production workers or only local dev?).
2. vlovich commented on Mar 5, 2023

   

   vlovich

   on Mar 5, 2023

   Last edited by vlovich

   ContributorAuthor

   More actions

   As you call out, your approach requires you have access to the promises passed to wait until. That's not solvable in all cases (e.g. you don't have access to the promise that ToucanJS will add when you captureException). Indeed I did something similar where I have a waitUntil wrapper that I do await ctx.waitUntil and all that waitUntil does is wait for the request to register itself with the test harness (in production it's an immediately resolved promise that's a no-op). The reason I filed this was I had problems around catching when Sentry and other observability events were likely done firing as those did not use my hacky waitUntil mechanism. I developed workarounds, but I'm not thrilled with how I did it.

   Doing this within the worker running within Workerd (i.e. on('exit', ...)) feels off - I would expect this to live at the Miniflare / workerd communication boundary instead (maybe some kind of capnp RPC or even as simple as Miniflare sending the write end fd of a pipe along with the request and then detecting when workerd closes it in response to RequestContext death). For example, if my script were to use too much memory in a waitUntil, the on('exit') approach wouldn't work if the runtime decided to proactively terminate the script. I don't think workerd specifically has that machinery but ideally this would be flexible to handle that scenario too I think.

   In my test setup, I have:

   1. Test harness code running on Node (outside of Miniflare/workerd)
   2. Production code running within workerd.

   I want 1 to know when 2 has finished all waitUntil. It can be coarse (i.e. wait until all request contexts are dead) or fine-grained (i.e. wait until this request context is dead). I get the concern about rejected waitUntil promises not causing test failures, but ultimately to me that seems like an application-level concern that workerd should be agnostic to. The test harness is better off written against the observable waitUntil side-effects I think rather than relying on detecting if the waitUntil internal to the code threw the exception (otherwise you get problems around "which waitUntil threw and is it one I cared about").

   I'm not looking at this in the context of production workers at all. e2e tests against production are a lot coarser about the kinds of assertions they can make today unless we can somehow build some way to have certain subrequests tunneled back out to the developer's machine. It might be cool to have that capability too as a way to level up the tests to be easier to write them against local + deployed simultaneously at the switch of a button, but that seems like something that needs a larger-scope project behind it to figure out all the nitty gritty details (e.g. subrequests for R2 + KV bindings probably shouldn't be intercepted so now you have to figure out which to passthrough and which to intercept).

   For now, aside from this issue, I'm generally extremely satisfied with the quality of test coverage from running Miniflare3 locally translating to observed quality when deployed. The major advantage would be to let EW run as a drop-in for workerd internally so we can get access to the production internal bindings vs me having to simulate then and orchestrate running a bunch of Miniflare3 instances by hand to simulate the CF network. We can discuss this point more internally if you like but in practice I have found my mocks to be possible to write against main workerd/Miniflare3 that this would mostly be a convenience.
3. vlovich commented on Mar 5, 2023

   

   vlovich

   on Mar 5, 2023

   Last edited by vlovich

   ContributorAuthor

   More actions

   To be clear, I think your allSettled trick might be a neat idea. One place I can see it failing is if you have a waitUntil that could get scheduled within a TransformStream you returned to the eyeball - there's no way for you to know when you are 100% done scheduling all waitUntil so you don't know when to "release" the lock in the test harness.

   Still, I think your approach combined with some test harness support would work in some more places than what I came up with so until this mechanism actually exists it's a better approach.