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.

Sign up for GitHub

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

Jump to bottom

feat(chain): use nextTick instead of setImmediate #1808

Merged
merged 1 commit into from Dec 2, 2019
Merged

feat(chain): use nextTick instead of setImmediate #1808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/chain.js
View file
Open in desktop
Expand Up @@ -112,7 +112,9 @@ Chain.prototype.run = function run(req, res, done) {

// all done or request closed
if (!handler || req.closed()) {
setImmediate(done, err, req, res);
process.nextTick(function nextTick() {
return done(err, req, res);
});
return;
}

Expand Down
8 changes: 4 additions & 4 deletions test/request.test.js
View file
Open in desktop
Expand Up @@ -227,15 +227,14 @@ test('should provide date when request started', function(t) {
// restifyDone is emitted at the same time when server's after event is emitted,
// you can find more comprehensive testing for `after` lives in server tests.
test('should emit restifyDone event when request is fully served', function(t) {
var clientDone = false;
var restifyDoneCalled = false;

SERVER.get('/', function(req, res, next) {
req.on('restifyDone', function(route, err) {
t.ifError(err);
t.ok(route);
setImmediate(function() {
t.ok(clientDone);
t.end();
restifyDoneCalled = true;
});
});

Expand All @@ -246,7 +245,8 @@ test('should emit restifyDone event when request is fully served', function(t) {
CLIENT.get('/', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
clientDone = true;
t.ok(restifyDoneCalled);
t.end();
});
});

Expand Down
14 changes: 7 additions & 7 deletions test/server.test.js
View file
Open in desktop
Expand Up @@ -1820,7 +1820,7 @@ test('calling next(false) should early exit from use handlers', function(t) {

SERVER.on('after', function() {
steps++;
t.equal(steps, 2);
t.equal(steps, 1);
t.end();
});

Expand Down Expand Up @@ -2111,7 +2111,7 @@ test('should increment/decrement inflight request count', function(t) {
CLIENT.get('/foo', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(SERVER.inflightRequests(), 1);
t.equal(SERVER.inflightRequests(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. Is this a result of CLIENT and SERVER sharing an event loop? Looking at this change, I don't see how it would cause a problem with our inflight request throttling. I could see the interaction between open request throttling and inflight request throttling changing with this PR, though I don't see a use case for both of those being turned on at the same time.

});
});

Expand All @@ -2135,14 +2135,14 @@ test('should increment/decrement inflight request count for concurrent reqs', fu
CLIENT.get('/foo1', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(SERVER.inflightRequests(), 1);
t.equal(SERVER.inflightRequests(), 0);
t.end();
});

CLIENT.get('/foo2', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(SERVER.inflightRequests(), 2);
t.equal(SERVER.inflightRequests(), 1);
});
});

Expand Down Expand Up @@ -2174,7 +2174,7 @@ test('should cleanup inflight requests count for 404s', function(t) {
CLIENT.get('/foo1', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(SERVER.inflightRequests(), 1);
t.equal(SERVER.inflightRequests(), 0);

CLIENT.get('/doesnotexist', function(err2, _2, res2) {
t.ok(err2);
Expand Down Expand Up @@ -2219,7 +2219,7 @@ test('should cleanup inflight requests count for timeouts', function(t) {
CLIENT.get('/foo2', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(SERVER.inflightRequests(), 2);
t.equal(SERVER.inflightRequests(), 1);
});
});

Expand Down Expand Up @@ -2862,7 +2862,7 @@ test('inflightRequest accounting stable with firstChain', function(t) {
for (var i = 0; i < results.length; i++) {
// The shed request should always be returned first, since it isn't
// handled by SERVER.get
if (i === 0) {
if (i === 1) {
t.equal(
results[i].statusCode,
413,
Expand Down