fix: explicitly destroy incoming stream when request ends#251
Conversation
|
Hi @yusukebe |
| app.post('/posts', (c) => { | ||
| return c.redirect('/posts') | ||
| }) | ||
| app.post('/no-body-consumed', (c) => { |
There was a problem hiding this comment.
Do these tests fail in the main? I've tried to do it, but these have not failed.
I want to know how we reproduce issue #246. I tried some cases. I can confirm that the case you pointed out with the Body Limit middleware has failed.
However, I couldn't find another case that failed due to the same issue. Is my understanding correct that only the case above fails?
There was a problem hiding this comment.
Hi @yusukebe
Thank you for pointing that out!
You're right, I originally added it in #250 to ensure coverage, but in this branch, it is indeed a “repeat of tests that were successful in main.”
As I mentioned in the following comment, this issue cannot be tested with supertest because it requires an API that is aware of sockets at a low level.
I will add another test, so please bear with me for a little longer.
|
Hi @usualoma ! Thank you for the PR. I left the comment. Though my understanding may be incorrect, can you check it? |
|
I have good news and bad news. Bad news first: This PR won't work with keepalive connections. Good news: After some effort, I figured out how to fix it and have a working solution tested and ready. New tests:
So, I'm closing this PR for now. Please wait a little longer. This is incredibly hard work. |
|
Oh... I have understood it. Thanks! |
2 participants
fixes #246
I tried various things, but I think the simplest solution is to call
incoming.destroy()for all requests (although this only happens whenreq[abortControllerKey]is present, i.e., when aRequestobject is generated).https://github.com/usualoma/node-server/blob/fba40a9130b68d25602f10f002b5ddc0970c9ddc/src/request.ts#L117-L123
With this change, even in cases where the “incomplete body loading” mentioned in the following comments of the issue occurs, the server will always close the incoming socket.
In existing apps, this change will cause
incoming.destroy(), which was not explicitly called before, to be called, but I don't think it will have any effect based on my testing.