[Feature Request] allow opt-out of workflow isolation

[neelance]

The bug #1432 just caused a lot of pain for us. It made our workers consume a lot of memory over time (and go OOM repeatedly) and made them go really slow over time, since every (implicit) call to new Promise was affected. It took me several weeks of investigating, giving up and then trying again on a different day to track down this root cause. This is because the issue only happened in production and most metrics that I could get from production did not clearly point to the issue. Even diffs on heap snapshots did not clearly point to AsyncLocalStorage because it contained a huge amount of other heap differences and AsyncLocalStorage did not catch my attention. I only got lucky that a CPU profile seemed odd and I noticed that the internals of Promise took much too long.

Our code was using new AsyncLocalStorage as a constant at the toplevel of a module. Normally such a constant only gets initialised once. It is quite unintuitive that with workflow isolation one has to consider that this code gets executed multiple times and thus reason through performance implications. #1432 plans to hide these implications, but it also mentions that maxCachedWorkflow might still be a performance issue.

This is not the first issue that we have due to the workflow isolation feature and I'm sorry, but I doubt that it will be the last. In theory workflow isolation seems like a good idea, but in practice it adds complexity that causes very uncommon issues. I want to point out that for example the Go SDK does not try to apply workflow isolation. Instead it expects the developer to write proper workflow code that respects deterministic execution requirements. I would rather teach my team to handle a transparent nondeterminism-error due to bad workflow code than to have issues like the one above. Therefore I would like to request the option to opt-out of workflow isolation (and instead opt-in to simplicity). I still appreciate the effort you've put into workflow isolation and its performance optimisations like reusable VMs.

[lukeramsden]

I'd like to echo this sentiment - while the fact that determinism can be "enforced" is very helpful early on, there are other mechanisms (such as replay testing) that more mature orgs can use that are less rigid at runtime. Integrating the workflow isolation with a projects build stack is also quite painful.

[mjameswh]

I totally understand the feeling, and I would personally also like to explore making sandboxing optional.

That is, however, a much larger effort than it may appear, which we can't prioritize just now. I would also point out that it is not clear to me at this point either getting rid of the Workflow sandbox would effectively result in less restrictions and lower technical complexity than what we have now. It may actually have the opposite effect.

The thing is that even though we generally present the sandbox as a way of isolating imports and global variables for determinism reasons, the sandbox also plays a second, more subtle but actually more important role: each Node's VMs come with a distinct event loop. That means that when the worker makes a call into the VM context, it is guaranteed that by the time that call returns, all outstanding microtasks belonging to that context will have been settled. This is how we know that a Workflow Task has completed, and how we know that operations are always replayed in the correct order.

Some languages make it possible to somehow customize scheduling of async completions; for example, in .Net, we created a custom task scheduler; for Python, that's a custom asyncio.AbstractEventLoop. In other languages, such as Java and Go, we have had to provide a completely distinct set of APIs to deal with async stuff inside of Workflow context (e.g. workflow.Go() instead of the go statement, wokrflow.Channel instead of Go's chan type, workflow.Selector instead of go's select statement, Promise instead of Java's Future and CompletableFuture, etc).

Node doesn't allow us to interfere with how its event loop works, like we do in .Net and Python. To some extent, it may be possible to rely on setImmediate() to determine that a Workflow Task has completed, but that would result in much weaker guarantees, especially if the goal is to have Workflow code coexist in the same execution environment as non-Workflow code. This is obviously an avenue that would have to be investigated, but should it indeed not provide the guarantees we need, the only alternative might be to have a completely distinct async APIs, similar to what we do in Java and Go. And if we reach that point, then we would no longer be able to use pure async constructs, but may have to transpile those into generators, just like it was common to do a few years ago. That means that in practice, we'd just effectively be replacing the current sandbox by a different type of sandbox. That's certainly not better.

Given those uncertainties, I think it is better for now to focus on making the sandbox better and more transparent, rather than avoiding it completely.

[neelance]

We just had another sudden breakage when upgrading the Temporal SDK.

TypeError: Symbol.asyncDispose is not defined.

To make things worse, the error does not occur when replaying the workflow locally, it only happens in production / with a full worker. I guess #1605 is throwing away our Symbol.asyncDispose polyfill, which we need due to nodejs/node#50745.

[mjameswh]

TypeError: Symbol.asyncDispose is not defined.

I guess #1605 is throwing away our Symbol.asyncDispose polyfill […].

Yeah, that's very likely related, though the symptoms are not exactly those I would expect. How are you loading the polfill?

I have work in progress to formalize the proper way to load polyfills; see #1743. The tricky thing is that polyfills that modify built-in globals need to be loaded early, when the workflow bundle gets loaded into the VM context (once, at worker startup time), rather than when your workflow.ts module actually gets executed (on every workflow start).

[neelance]

For now I have moved the polyfill directly into the injected code of reusable-vm.js via patch. This seems to work good enough.

The thing is that even though we generally present the sandbox as a way of isolating imports and global variables for determinism reasons, the sandbox also plays a second, more subtle but actually more important role: each Node's VMs come with a distinct event loop. That means that when the worker makes a call into the VM context, it is guaranteed that by the time that call returns, all outstanding microtasks belonging to that context will have been settled. This is how we know that a Workflow Task has completed, and how we know that operations are always replayed in the correct order.

This makes sense. Still, this feature could be used without all the isolation efforts, couldn't it? You would only need to initialise maxConcurrentWorkflowTaskExecutions VMs once and then just plainly use them for executing workflow tasks, without any additional isolation efforts whatsoever.

Yeah, that's very likely related, though the symptoms are exactly those I would expect.

As a user of the SDK, I want to mention again that such symptoms (and debugging and patching them) are a negative impact for us and it does not seem to me that the advantages of workflow isolation outweigh them to result in a positive tradeoff.

[mjameswh]

This makes sense. Still, this feature could be used without all the isolation efforts, couldn't it?

Of course, and that's already supported. Just add reuseV8Context: false to your Worker options.

A new VM context gets created for each Workflow execution, so there's no for context bookeeping and switching. That's actually how the sandbox was initially designed, with a much simpler execution model, and therefore, less surprises and complications. The "Reuse V8 Context" optimization came in later, resulting in 2/3 reduction in memory usage and between 1/3 and 1/2 reduction in CPU usage, according to our internal stress tests and confirmed by many users. That's why we made that execution model the default, but the original executor was kept around, for those cases where restrictions of the "Reuse V8 Context" executor are too cumbersome.

Yeah, that's very likely related, though the symptoms are exactly those I would expect.

As a user of the SDK, I want to mention again that such symptoms (and debugging and patching them) are a negative impact for us and it does not seem to me that the advantages of workflow isolation outweigh them to result in a positive tradeoff.

Once again, I fully sympathize. But exploring alternative execution models is simply not something we can prioritize at this time.

[neelance]

Of course, and that's already supported. Just add reuseV8Context: false to your Worker options.

A new VM context gets created for each Workflow execution, so there's no for context bookeeping and switching.

This is not what I meant. I was talking about creating new VM contexts only on startup of the worker and then reusing them for the whole lifetime of the worker. This would attempt no workflow isolation and only use them for the microtask handling that you mentioned.

[neelance]

Once again, I fully sympathize. But exploring alternative execution models is simply not something we can prioritize at this time.

Thanks. I appreciate that you take the time for this conversation. I'll still keep a log of the issues that we see in this thread, so it can be used as a reference. It is up to you to decide which way to go.