-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Add a test about socketOnDrain where needPause is false #17654
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
Changes from 1 commit
4bb861b
959ea3e
73c682c
6b021c5
9b5118c
1231793
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const net = require('net'); | ||
const http = require('http'); | ||
|
||
let requestReceived = 0; | ||
|
||
const server = http.createServer(function(req, res) { | ||
const id = ++requestReceived; | ||
const enoughToDrain = req.connection.writableHighWaterMark; | ||
const body = 'x'.repeat(enoughToDrain); | ||
|
||
if (id === 1) { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/hightWaterMark/highWaterMark There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for always review. |
||
})); | ||
res.write(body); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add an assert here as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got it. I added a assertion. |
||
} else { | ||
// Case of needParse = true | ||
const resume = req.connection.parser.resume.bind(req.connection.parser); | ||
req.connection.parser.resume = common.mustCall((...args) => { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/hightWaterMark/highWaterMark |
||
return resume(...args); | ||
}); | ||
assert(!res.write(body), 'res.write must be fail because it will exceed ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got it. I updated error message. |
||
'hightWaterMark on this call'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/hightWaterMark/highWaterMark |
||
} | ||
res.end(); | ||
}).on('listening', () => { | ||
const c = net.createConnection(server.address().port, () => { | ||
c.write('GET / HTTP/1.1\r\n\r\n'); | ||
c.write('GET / HTTP/1.1\r\n\r\n'); | ||
c.end(); | ||
}); | ||
|
||
c.on('data', () => {}); | ||
c.on('end', () => { | ||
server.close(); | ||
}); | ||
}) | ||
.listen(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this go on the previous line or just be its own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a description of the test here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review.
I added a description. Please check it again.