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

feat(chain): schedule handlers to the next tick #1798

Merged
merged 1 commit into from
Jul 31, 2019
Merged

feat(chain): schedule handlers to the next tick #1798

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,9 @@ function call(handler, err, req, res, _next) {
return;
} else if (!hasError && arity < 4) {
// request-handling middleware
handler(req, res, next);
process.nextTick(function nextTick() {
Copy link

@kenashcraft kenashcraft Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Have you run the benchmarks with this PR? Consider this implementation instead:
process.nextTick(handler, req, res, next);
Looking at the implementation of nextTick with Julien, it isn't clear which would be faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I run, looks like no effect

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal would be cleaner flamegraphs not faster execution.

Copy link
Member

Choose a reason for hiding this comment

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

Is this enough to reset the FG stack? Or do we need setImmediate?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @misterdjules

Copy link
Contributor

Choose a reason for hiding this comment

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

What do we mean by "FG stack"? If the question is whether the handler call's stack frame would not have the previous handler as a parent stack frame, the answer is yes, it would not have the previous handler as a parent stack frame.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah Peter clarified that FG is for Flame Graphs :)

handler(req, res, next);
});
return;
}

Expand Down
20 changes: 12 additions & 8 deletions test/chain.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';
/* eslint-disable func-names */

var domain = require('domain');
var Chain = require('../lib/chain');

if (require.cache[__dirname + '/lib/helper.js']) {
Expand Down Expand Up @@ -184,7 +185,7 @@ test('onceNext prevents double next calls', function(t) {
});

test('throws error for double next calls in strictNext mode', function(t) {
var doneCalled = 0;
t.expect(1);
var chain = new Chain({
strictNext: true
});
Expand All @@ -194,7 +195,15 @@ test('throws error for double next calls in strictNext mode', function(t) {
next();
});

try {
var testDomain = domain.create();

testDomain.on('error', function onError(err) {
t.equal(err.message, "next shouldn't be called more than once");
testDomain.exit();
t.done();
});

testDomain.run(function run() {
chain.run(
{
startHandlerTimer: function() {},
Expand All @@ -206,14 +215,9 @@ test('throws error for double next calls in strictNext mode', function(t) {
{},
function(err) {
t.ifError(err);
doneCalled++;
t.equal(doneCalled, 1);
t.done();
}
);
} catch (err) {
t.equal(err.message, "next shouldn't be called more than once");
}
});
});

test('calls req.startHandlerTimer', function(t) {
Expand Down