Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not swallow calls to res.end() #188

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

martinslota
Copy link

@martinslota martinslota commented Aug 13, 2024

Background

In our application we tend to rely on res.end() calls to perform cleanup. Specifically, we intercept those calls, similarly to how the same is done in the compression middleware, and take custom actions before calling the original res.end function.

Recently we found out that depending on middleware order, our cleanup code either gets called reliably or it does not. Additional investigation revealed that it is the compression middleware that sometimes "swallows" calls to res.end, which is to say that even though a route handler calls res.end, the middleware never calls the original res.end function. In particular, this happens when the client cuts the connection before consuming the compressed response because the following happens:

  1. Since the connection is cut, bytes cannot continue to be written into the response, and the compressed stream gets paused.
  2. Once the route handler calls res.end, the compression middleware does not call the original res.end function and instead calls stream.end.
  3. Since the data in the stream is not entirely consumed, the end event does not get emitted and the handler attached to it never calls the original res.end function.

We considered using the finish or prefinish events instead of wrapping res.end calls, but these do not get emitted either, likely because OutgoingMessage.end never gets called either.

Change

  1. Adds tests covering a few scenarios under which the original res.end function does not get called with the code on master
  2. Uses on-finished to detect client disconnect and call the original res.end function - though this is only done if the replacement res.end wrapper had already been called, or if it is currently being called
  3. Due to the added call sites for the original res.end function, the changed code also ensures that we never call the function more than once

Overall, the change should ensure that res.end calls do not get swallowed by the compression middleware.

Acknowledgement

Lots of the ideation around this change as well as the test code originate from @papandreou - many thanks for all the help! 🙇

Comment on lines +122 to +124
if (onFinished.isFinished(this)) {
return endOnce.call(this)
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition takes care of situations where a route handler calls res.end() after the response is considered "finished". This is often due to the client having moved on before the response is ready for them.

Comment on lines +232 to +236
onFinished(res, function onFinished () {
if (ended) {
endOnce.call(res)
}
})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition takes care of situations where a response becomes "finished" soon after res.end() gets called by a route handler. Without this code, if the events occur close enough to one another, the stream may (at least in principle) get stuck in a "paused" state, never emitting the end event that would normally invoke the original res.end function.

Comment on lines +680 to +688
setTimeout(function () {
server.close(function () {
if (originalResEndCalledTimes === 1) {
done()
} else {
done(new Error('The original res.end() was called ' + originalResEndCalledTimes + ' times'))
}
})
}, 5)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for the condition to assert could be extracted into a generic helper, and it could also poll a couple of times if that is desired. I left this code in its naked form for now, and it is also duplicated in the other added test cases. I'm happy to improve this further as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant