Handling stream errors in a TransformStream transformer

[h4l]

The background to this is that I found a bug in deno's TextDecoderStream implementation that results in it failing to clean up a resource it holds if its stream pipeline aborts with an error. The implementation holds a TextDecoder which it uses in streaming mode. The TextDecoder holds a resource handle to a native object handling text decoding for it, which it closes when decode() is called without {stream: true}. When the deno TextDecoderStream implementation's transformer gets a flush() call, it calls decode() to close the TextDecoder. However, if the stream aborts, flush() is not called, so the native resource handle is not closed, and gets leaked.

I've looked through the Streams spec, and as I understand it there's no built-in way for a transformer to be notified of a stream error.
It is possible to work around this as an API user, as I mention in the deno issue:

I played around with the Streams API a bit and came up with a fairly straightforward way to implement a TransformStream whose Transformer gets notified of stream aborts. Basically two parts:

• A WritableStream can be monitored for errors by wrapping it with another WritableStream that opens a reader on the monitored stream, exposes the reader's closed promise (which rejects if the monitored stream is aborted), and forwards start/write/close/abort calls to the monitored stream.
  That looks like this: https://deno.land/x/[email protected]/shutdown_monitor_writable_stream.ts
• Then a TransformStream can react to stream aborts by monitoring its writable side with the monitor stream, and using the closed promise to be notified when the stream aborts.
  That looks like this: https://deno.land/x/[email protected]/shutdown_aware_transform_stream.ts#L98

Although I say "fairly straightforward", it's not exactly trivial. And another alternative of not using TransformStream and instead tying together a readable and writable stream manually to create a (readable, writable) pair is even more fiddly to do correctly.

As an API user, it seems like there should be an idiomatic way to handle stream errors in a transformer. The underlying sink of a WritableStream can do so either with its abort() method, or via the AbortSignal on WritableStreamDefaultController's signal property.

What do you think about giving transformers similar capabilities to handle aborts as underlying sinks?

Even just giving TransformStreamDefaultController an AbortSignal would be helpful (I presume that's simpler to spec than a method on transformer, as it can't affect the error propagation behaviour). Although I suppose a method would allow for asynchronous cleanup...

[domenic]

I think #636, extended to transform streams (see some discussion in #1026) is probably the right solution here...

[h4l]

A finally() method guaranteed to be called (like the opposite of start()) would be good. I do find it a bit surprising that abort() is not called when a WritableStream throws from one of its underlying sink methods, or calls controller.error().

[MattiasBuelens]

I do find it a bit surprising that abort() is not called when a WritableStream throws from one of its underlying sink methods, or calls controller.error().

We assume that controller.error() is only called from within the underlying sink itself, so it wouldn't be useful to have error() call yet another sink method.

Similarly, we assume that errors thrown (or promises rejected) from within a sink method were already handled by that sink method (e.g. with a try..catch).

abort() is reserved for an error that was injected outside of the sink, i.e. by the writer.

---

That said, I do agree with the OP: cancelling the readable end and/or aborting the writable end of a transform stream should call some sort of finally() or close() method on the underlying transformer, so it can clean up any held resources. 👍

[domenic]

Yeah, I think this is especially acute for transformers. In the previous discussions it was more "it's awkward to know where to put your cleanup logic"; for transforms it seems like "there's nowhere to put your cleanup logic".

[lucacasonato]

@domenic That's exactly right. This is actually getting more and more pressing by the day. I'll see if we can allocate some resources to open a PR for this. I guess the proposed solution right now is to add a Transformer#finally callback?

[jasnell]

Definite +1 to a finally algorithm on the transformer.

[kanongil]

Wow, I'm surprised about this omission, making it impossible for TransformStream sources to cleanup state on external stream errors / aborts. Hope this gets resolved, as the workarounds seem quite cumbersome.