Describe the bug
If an error occurs while sending an error response in a request handler() it remains unhandled if there's no onerror handler on the protocol (which is the default)
This means that nothing is returned to the client and the promise rejection goes unhandled. On Cloudflare Workers, this will result in an error like this: A hanging Promise was canceled. This happens when the worker runtime is waiting for a Promise from JavaScript to resolve, but has detected that the Promise cannot possibly ever resolve because all code and events related to the Promise's I/O context have already finished., and on Node.js, I suspect this would result in an unhandledRejection event though I haven't confirmed this – it may just be that the request stays open and eventually times out.
The problem is it's very hard to know exactly what's happened (or even that adding an onerror handler may help)
Expected behavior
Currently the protocol fires off Promises without awaiting them. Messages are processed in a loop without waiting, which means they all get processed in parallel, with no limits or waiting for execution. And the _onrequest handler creates a Promise without awaiting or returning it.
This kind of event handling becomes difficult to manage unless all event handlers ultimately resolve (including error handling). In the case of unhandled rejections, it's often not clear where the rejection happened, if it's even logged / managed at all – the error could just disappear into the ether if there's no error handler.
It also means it's very easy for errors to be handled multiple times, and if each handler writes a message on the transport, you then end up with Cannot write headers after they are sent to the client.
Ideally, install default error handlers that can handle an error occurring anywhere and won't leave unhandled promises/rejections. Having a default handler that at least does console.error would ensure that users should see the error, even if it's not handled gracefully.
Longer term I'd love to see this refactored to use async/await instead of events – it should be much easier to reason about, much easier to handle concurrency and flow control (eg message processing), and much easier to ensure errors or responses aren't sent over the wire multiple times.
Describe the bug
If an error occurs while sending an error response in a request
handler()it remains unhandled if there's noonerrorhandler on the protocol (which is the default)This means that nothing is returned to the client and the promise rejection goes unhandled. On Cloudflare Workers, this will result in an error like this:
A hanging Promise was canceled. This happens when the worker runtime is waiting for a Promise from JavaScript to resolve, but has detected that the Promise cannot possibly ever resolve because all code and events related to the Promise's I/O context have already finished., and on Node.js, I suspect this would result in anunhandledRejectionevent though I haven't confirmed this – it may just be that the request stays open and eventually times out.The problem is it's very hard to know exactly what's happened (or even that adding an
onerrorhandler may help)Expected behavior
Currently the protocol fires off Promises without awaiting them. Messages are processed in a loop without waiting, which means they all get processed in parallel, with no limits or waiting for execution. And the
_onrequesthandler creates a Promise without awaiting or returning it.This kind of event handling becomes difficult to manage unless all event handlers ultimately resolve (including error handling). In the case of unhandled rejections, it's often not clear where the rejection happened, if it's even logged / managed at all – the error could just disappear into the ether if there's no error handler.
It also means it's very easy for errors to be handled multiple times, and if each handler writes a message on the transport, you then end up with
Cannot write headers after they are sent to the client.Ideally, install default error handlers that can handle an error occurring anywhere and won't leave unhandled promises/rejections. Having a default handler that at least does
console.errorwould ensure that users should see the error, even if it's not handled gracefully.Longer term I'd love to see this refactored to use async/await instead of events – it should be much easier to reason about, much easier to handle concurrency and flow control (eg message processing), and much easier to ensure errors or responses aren't sent over the wire multiple times.