Drain insert HTTP response #203
Conversation
|
|
…ng it. Calling destroy eventually closed the socket even if keep-alive was set.
varrocs
force-pushed
the
drain_http_response
branch
from
8a3cc87 to
6e3efbd
Compare
| async drainHttpResponse(stream: Stream.Readable): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| function dropdata() { | ||
| // We don't care about the data |
There was a problem hiding this comment.
Do we need to add resolve() here? Also, minor nit: dropData
There was a problem hiding this comment.
Do we need to add resolve() here?
No. This function will be called when data is available but we just want to throw away the data, we don't care.
There was a problem hiding this comment.
minor nit: dropData
Fixed
If we want to test it, we need to spy on sockets somehow (with max connections set to 1); I am afraid there is no other way. (please don't mind failing Cloud tests for now; for some reason, Actions cannot fetch the secret properly for the community PRs). |
Well, let's say I don't insist on testing this. |
So it's me asking if you accept the pull request without writing tests to test if the consecutive requests are going on the same socket |
|
@varrocs, I think we can live without such a test for now cause nothing else is broken, and we have a specific test for consecutive inserts with one socket, IIRC (and they pass). I'd only ask you to confirm that with this change, the TCP dump looks like the fixed version mentioned here: #202 (comment) |
|
Yes you have such test (with a comment that saying that how strange it is that the socket mostly reopened even with keep alive) |
In case of an insert call drain the HTTP response instead of destroying it.
Summary
Calling destroy eventually closed the socket even if keep-alive was set. This was a performance limitation.
#202
Checklist
Delete items not relevant to your PR:
Note: I don't know how to test this
Note: I don't know if it applies to my change