Add a test about socketOnDrain where needPause is false#17654
Add a test about socketOnDrain where needPause is false#17654Leko wants to merge 6 commits intonodejs:masterfrom Leko:test_http_socketondrain
Conversation
Our test suite does not check when needPause becomes false. I add a test to improve coverage.
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
LGTM with some nits, good work!
| 'hightWaterMark'); | ||
| return resume(...args); | ||
| }); | ||
| assert(!res.write(body), 'res.write must be fail because it will exceed ' + |
There was a problem hiding this comment.
I think this should be res.write must return false
There was a problem hiding this comment.
I got it. I updated error message.
| assert(req.connection._paused, '_paused must be true because it exceeds' + | ||
| 'hightWaterMark by second request'); | ||
| })); | ||
| res.write(body); |
There was a problem hiding this comment.
can you add an assert here as well?
There was a problem hiding this comment.
I got it. I added a assertion.
| const assert = require('assert'); | ||
| const net = require('net'); | ||
| const http = require('http'); | ||
|
|
There was a problem hiding this comment.
Can you add a description of the test here?
There was a problem hiding this comment.
Thank you for your review.
I added a description. Please check it again.
apapirovski
left a comment
apapirovski
left a comment
There was a problem hiding this comment.
LGTM, just some nits
| // Case of needParse = false | ||
| req.connection.once('pause', common.mustCall(() => { | ||
| assert(req.connection._paused, '_paused must be true because it exceeds' + | ||
| 'hightWaterMark by second request'); |
There was a problem hiding this comment.
s/hightWaterMark/highWaterMark
There was a problem hiding this comment.
Thank you for always review.
I'm sorry for my stupid mistake...
I fixed all typo.
| server.close(); | ||
| }); | ||
| }) | ||
| .listen(0); |
There was a problem hiding this comment.
Can this go on the previous line or just be its own server.listen(0); statement? It's a bit awkward to follow right now due to the lack of indentation.
There was a problem hiding this comment.
I got it.
I split statement as }); and server.listen(0);.
| return resume(...args); | ||
| }); | ||
| assert(!res.write(body), 'res.write must be fail because it will exceed ' + | ||
| 'hightWaterMark on this call'); |
There was a problem hiding this comment.
s/hightWaterMark/highWaterMark
| const paused = req.connection._paused; | ||
| assert(!paused, '_paused must be false because it become false by ' + | ||
| 'socketOnDrain when outgoingData falls below ' + | ||
| 'hightWaterMark'); |
There was a problem hiding this comment.
s/hightWaterMark/highWaterMark
`res.write must be fail` to `res.write must return false`
|
Landed as a364e7e. |
Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: #17654 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: #17654 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: #17654 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: #17654 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Adds a tests that checks if we can start reading again from a socket backing an incoming http request. PR-URL: #17654 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
5 participants
This PR closes #17051
@mcollina Would you please review my changes ?
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test