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

directly invoke error middleware #1085

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@vincentLiuxiang

vincentLiuxiang commented Sep 9, 2016

directly invoke error middleware avoid recursiVe traversal the whole app.stack

流香
@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 9, 2016

Contributor

This will invoke error middleware that were declared before that middleware that had the error, which is a very large change in behavior. Can you explain in more detail your proposal? Can you add tests?

Contributor

dougwilson commented Sep 9, 2016

This will invoke error middleware that were declared before that middleware that had the error, which is a very large change in behavior. Can you explain in more detail your proposal? Can you add tests?

@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Sep 9, 2016

hi, @dougwilson I have explained my proposal in Issues 1084, and I have checked all tests case base on connect tests, and I will continue to add test cases :)

vincentLiuxiang commented Sep 9, 2016

hi, @dougwilson I have explained my proposal in Issues 1084, and I have checked all tests case base on connect tests, and I will continue to add test cases :)

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 9, 2016

Contributor

I understand the other issue, I'm only asking to explain why you made this perform an improvement a breaking change instead of backwards compatible. If improving the performance cannot be done in a backwards compatible wat, then you are really proposing a Chang in how connect error handling works, not simply a performance improvement.

Yes, we are missing a test for what you broke with tour change. I tried to explain what you broke in words above, but I will try to get a test checked into master so you can see what you are breaking that way.

Contributor

dougwilson commented Sep 9, 2016

I understand the other issue, I'm only asking to explain why you made this perform an improvement a breaking change instead of backwards compatible. If improving the performance cannot be done in a backwards compatible wat, then you are really proposing a Chang in how connect error handling works, not simply a performance improvement.

Yes, we are missing a test for what you broke with tour change. I tried to explain what you broke in words above, but I will try to get a test checked into master so you can see what you are breaking that way.

@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Sep 9, 2016

@dougwilson I just simply thought an idea of improving performance. According to my understanding, add a app.errware does not affect the users to use connect, it totally backwards compatible.

As a very popular project, you do need to be cautious to merge code for connect.No mater how, it's my honor to communicate with you.

vincentLiuxiang commented Sep 9, 2016

@dougwilson I just simply thought an idea of improving performance. According to my understanding, add a app.errware does not affect the users to use connect, it totally backwards compatible.

As a very popular project, you do need to be cautious to merge code for connect.No mater how, it's my honor to communicate with you.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 9, 2016

Contributor

It's no problem! So my understanding here is that you simply accidentally broke the existing behavior and since we are missing some tests you didn't notice (sorry!). I'm going to add a few more tests and ping you here so you can run your PR against the new tests and make any necessary adjustments to the PR to work against the tests, if that is good to you :)

Contributor

dougwilson commented Sep 9, 2016

It's no problem! So my understanding here is that you simply accidentally broke the existing behavior and since we are missing some tests you didn't notice (sorry!). I'm going to add a few more tests and ping you here so you can run your PR against the new tests and make any necessary adjustments to the PR to work against the tests, if that is good to you :)

@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Sep 9, 2016

sorry @dougwilson I am very new in github to commit code and I am not very experienced to write test cases. I just put forward my idea, you can determine its rationality 💯 :)

vincentLiuxiang commented Sep 9, 2016

sorry @dougwilson I am very new in github to commit code and I am not very experienced to write test cases. I just put forward my idea, you can determine its rationality 💯 :)

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 9, 2016

Contributor

It's no problem! So I just checked in the following test: 69a12f8 that demonstrates how your proposed changes would break current apps. You don't need to add tests, but would you be able to make a new proposal that works against the new test as well?

Contributor

dougwilson commented Sep 9, 2016

It's no problem! So I just checked in the following test: 69a12f8 that demonstrates how your proposed changes would break current apps. You don't need to add tests, but would you be able to make a new proposal that works against the new test as well?

流香 added some commits Sep 9, 2016

流香
流香
@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Sep 9, 2016

hi @dougwilson I have solve my bug , and pass your test case. then , I will make a benchmark test.

vincentLiuxiang commented Sep 9, 2016

hi @dougwilson I have solve my bug , and pass your test case. then , I will make a benchmark test.

@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Sep 10, 2016

hi @dougwilson this is my benchmark

  • My environment:macbook air

in order to start my test, I just commented below code in connect:
because of finalhandler will response for client

...   
// all done
if (!layer) {
  // defer(done, err);  //commented in my test
  return;
}
...
  • my test scripts:
var Benchmark = require('benchmark');
var appNew = require('../../../../github/connect')();
var appOld = require('connect')();
var http = require('http');

var suite = new Benchmark.Suite;

function test (app,pre,errpre,next,errnext) {
  for (var i = 0; i < pre; i++) {
    app.use((req,res,next) => {
      next();
    });
  }

  for (var i = 0; i < errpre; i++) {
    app.use((err,req,res,next) => {
      // res.end(err.message);
    })
  }

  for (var i = 0; i < next; i++) {
    app.use((req,res,next) => {
      next(new Error('error'));
    });
  }

  for (var i = 0; i < errnext; i++) {
    app.use((err,req,res,next) => {

    })
  }

  return app;
}

/**
 * a bad benchmark case for appNew
 * if app does not have error-middleware,
 * and it will has no Performance optimization
 */
// appNew = test(appNew,50);
// appOld = test(appOld,50);

/**
 * a beneficial benchmark case for appNew
 * if app has error-middleware,
 * the Performance optimization rely on the number of error-middleware
 * and it's position when invoke app.use
 */
appNew = test(appNew,0,50,50,1);
appOld = test(appOld,0,50,50,1);

var req = {url:'/'};
var res = {};


console.log('start testing ...');

suite
.add('connectOld', function() {
  appOld(req,res);
})
.add('connectNew', function() {
  appNew(req,res)
})
.on('cycle', function(event) {
  console.log(String(event.target));
})
.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
})
.run({ 'async': true });
  • benchmark result:

appNew = test(appNew,50);
appOld = test(appOld,50);
the benchmark is almost.

-> node benchmark.js
start testing ...
connectOld x 48,994 ops/sec ±4.75% (64 runs sampled)
connectNew x 51,117 ops/sec ±1.67% (81 runs sampled)
Fastest is connectNew,connectOld

appNew = test(appNew,0,50,50,1);
appOld = test(appOld,0,50,50,1);
the performance of appNew is 5 times as appOld;

-> node benchmark.js
start testing ...
connectOld x 20,925 ops/sec ±3.47% (74 runs sampled)
connectNew x 106,560 ops/sec ±1.53% (83 runs sampled)
Fastest is connectNew

vincentLiuxiang commented Sep 10, 2016

hi @dougwilson this is my benchmark

  • My environment:macbook air

in order to start my test, I just commented below code in connect:
because of finalhandler will response for client

...   
// all done
if (!layer) {
  // defer(done, err);  //commented in my test
  return;
}
...
  • my test scripts:
var Benchmark = require('benchmark');
var appNew = require('../../../../github/connect')();
var appOld = require('connect')();
var http = require('http');

var suite = new Benchmark.Suite;

function test (app,pre,errpre,next,errnext) {
  for (var i = 0; i < pre; i++) {
    app.use((req,res,next) => {
      next();
    });
  }

  for (var i = 0; i < errpre; i++) {
    app.use((err,req,res,next) => {
      // res.end(err.message);
    })
  }

  for (var i = 0; i < next; i++) {
    app.use((req,res,next) => {
      next(new Error('error'));
    });
  }

  for (var i = 0; i < errnext; i++) {
    app.use((err,req,res,next) => {

    })
  }

  return app;
}

/**
 * a bad benchmark case for appNew
 * if app does not have error-middleware,
 * and it will has no Performance optimization
 */
// appNew = test(appNew,50);
// appOld = test(appOld,50);

/**
 * a beneficial benchmark case for appNew
 * if app has error-middleware,
 * the Performance optimization rely on the number of error-middleware
 * and it's position when invoke app.use
 */
appNew = test(appNew,0,50,50,1);
appOld = test(appOld,0,50,50,1);

var req = {url:'/'};
var res = {};


console.log('start testing ...');

suite
.add('connectOld', function() {
  appOld(req,res);
})
.add('connectNew', function() {
  appNew(req,res)
})
.on('cycle', function(event) {
  console.log(String(event.target));
})
.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
})
.run({ 'async': true });
  • benchmark result:

appNew = test(appNew,50);
appOld = test(appOld,50);
the benchmark is almost.

-> node benchmark.js
start testing ...
connectOld x 48,994 ops/sec ±4.75% (64 runs sampled)
connectNew x 51,117 ops/sec ±1.67% (81 runs sampled)
Fastest is connectNew,connectOld

appNew = test(appNew,0,50,50,1);
appOld = test(appOld,0,50,50,1);
the performance of appNew is 5 times as appOld;

-> node benchmark.js
start testing ...
connectOld x 20,925 ops/sec ±3.47% (74 runs sampled)
connectNew x 106,560 ops/sec ±1.53% (83 runs sampled)
Fastest is connectNew
@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 10, 2016

Contributor

Thanks, @vincentLiuxiang , I'll play around with the benchmark. Your benchmark that shows the improvement doesn't make any sense though; it is creating an app that has 50 error handlers, then 50 middleware,. Why would any real-world app have 50 error handlers before even the first middleware? No error handler before at least one middleware would ever even do anything. Are there even apps that chain that many error handlers out there?

Contributor

dougwilson commented Sep 10, 2016

Thanks, @vincentLiuxiang , I'll play around with the benchmark. Your benchmark that shows the improvement doesn't make any sense though; it is creating an app that has 50 error handlers, then 50 middleware,. Why would any real-world app have 50 error handlers before even the first middleware? No error handler before at least one middleware would ever even do anything. Are there even apps that chain that many error handlers out there?

@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Sep 10, 2016

sorry @dougwilson ,maybe i gave a inappropriate example. look at the below example,50 non-error middleware and 1 error-middleware:

appNew = test(appNew,0,0,50,1);
appOld = test(appOld,0,0,50,1);

-> node benchmark.js
start testing ...
connectOld x 35,875 ops/sec ±5.24% (82 runs sampled)
connectNew x 145,842 ops/sec ±3.25% (79 runs sampled)
Fastest is connectNew
  • I just optimized my code, the non-error-middleware don't need to recursiVe traversal the whole app.errware, just move to invoke the Behind error-middleware
    if (err) {
      layer = errware[errIndex++];
      if (layer) index = layer.index;
    } else {
      layer = stack[index++];
      if (layer) errIndex = layer.index;
    }
    // all done
    if (!layer) {
      // defer(done, err);
      return;
    }
   ...

  if (handle.length === 4) {
    this.errware.push({ route: path, handle: handle, index:this.stack.length });
  } else {
    this.stack.push({ route: path, handle: handle, index:this.errware.length });
  }

vincentLiuxiang commented Sep 10, 2016

sorry @dougwilson ,maybe i gave a inappropriate example. look at the below example,50 non-error middleware and 1 error-middleware:

appNew = test(appNew,0,0,50,1);
appOld = test(appOld,0,0,50,1);

-> node benchmark.js
start testing ...
connectOld x 35,875 ops/sec ±5.24% (82 runs sampled)
connectNew x 145,842 ops/sec ±3.25% (79 runs sampled)
Fastest is connectNew
  • I just optimized my code, the non-error-middleware don't need to recursiVe traversal the whole app.errware, just move to invoke the Behind error-middleware
    if (err) {
      layer = errware[errIndex++];
      if (layer) index = layer.index;
    } else {
      layer = stack[index++];
      if (layer) errIndex = layer.index;
    }
    // all done
    if (!layer) {
      // defer(done, err);
      return;
    }
   ...

  if (handle.length === 4) {
    this.errware.push({ route: path, handle: handle, index:this.stack.length });
  } else {
    this.stack.push({ route: path, handle: handle, index:this.errware.length });
  }
流香
@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Oct 20, 2016

Contributor

Awesome, love it. Unfortunately this is altering the contents of app.stack, which a lot of modules, examples, and SO answers use to remove middleware, which this totally breaks. I definitely want to improve performance, but it seems very disruptive to everything. Perhaps if we first add APIs to connect that does what people are using the direct app.stack manipulation for, it will actually give them a way to not touch the structure we are trying to change here, idk.

Contributor

dougwilson commented Oct 20, 2016

Awesome, love it. Unfortunately this is altering the contents of app.stack, which a lot of modules, examples, and SO answers use to remove middleware, which this totally breaks. I definitely want to improve performance, but it seems very disruptive to everything. Perhaps if we first add APIs to connect that does what people are using the direct app.stack manipulation for, it will actually give them a way to not touch the structure we are trying to change here, idk.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 13, 2017

Contributor

Hi @vincentLiuxiang does closing this PR mean you're not interested in providing the APIs to enable the landing of this PR as mentioned above, or... ?

Contributor

dougwilson commented Feb 13, 2017

Hi @vincentLiuxiang does closing this PR mean you're not interested in providing the APIs to enable the landing of this PR as mentioned above, or... ?

@vincentLiuxiang

This comment has been minimized.

Show comment
Hide comment
@vincentLiuxiang

vincentLiuxiang Feb 13, 2017

Hi @dougwilson, I have realized my idea via golang. As you said, this PR will totally change app.stack. I am a deep user of connect, some of my work used app.stack directly and I quite agree with that this PR is disruptive. A lot of modules depend on connect (include my work), the better way maybe start a new project.

vincentLiuxiang commented Feb 13, 2017

Hi @dougwilson, I have realized my idea via golang. As you said, this PR will totally change app.stack. I am a deep user of connect, some of my work used app.stack directly and I quite agree with that this PR is disruptive. A lot of modules depend on connect (include my work), the better way maybe start a new project.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 17, 2017

Contributor

Gotcha, makes sense. Thanks for the reply!

Contributor

dougwilson commented Feb 17, 2017

Gotcha, makes sense. Thanks for the reply!

@dougwilson dougwilson reopened this Feb 17, 2017

@dougwilson dougwilson closed this Feb 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment