Improve expect 100-continue handling #4488
Conversation
|
The node 18 test failures are not related to this patch. It seems that it has been updated to no longer process requests after the listener has been closed. |
kanongil
force-pushed
the
better-continue
branch
from
ac81404
to
91667c5
Compare
| if (request._isPayloadPending) { | ||
| await internals.drain(request); | ||
| } | ||
| await internals.drain(request); |
There was a problem hiding this comment.
Do we have a guarantee that there's something to drain? I feel that this function is too brittle, should we use stream.finished to make sure all scenarios are covered?
There was a problem hiding this comment.
That is not a concern from this PR, right?
I tend to agree. The current drain() logic requires that the stream is active to work. An already closed stream would stall forever.
I would be wary to rely on stream.finished(), as I have seen streams that causes issues with it in the past.
There was a problem hiding this comment.
Not from this PR no, but removing that conditional made me look further whether it was solid or not, so I hope there's something to consume there, otherwise it's indeed stalled.
There was a problem hiding this comment.
FYI, draining might not be the best approach, but I retained the current behaviour.
The alternative is to just .destroy() the stream, which should work fine, but for HTTP/1 it will also destroy the connection. Draining is more graceful, but can needlessly use up bandwidth. It works best for small pending payloads.
This fixes 2 issues in the existing handling.
The most serious is the one exposed in the "handles expect 100-continue on undefined routes". These routes bypass most of the normal processing, meaning that the server never signals for the client to continue. This causes an infinite stall when
internals.drain()is called, since the client hasn't been instructed to send data.The other issue, is that when overriding
request.payload, hapi will still request the client to continue sending the payload, even though it will never be used.The first test is just to verify that hapi will skip requesting the client to continue when processing is aborted before the "payload processing" stage.