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

next(new Error(....)),should directly invoke the error middleware #1084

Closed
vincentLiuxiang opened this Issue Sep 9, 2016 · 1 comment

Comments

Projects
None yet
2 participants
@vincentLiuxiang

vincentLiuxiang commented Sep 9, 2016

hi all,
in current connect version, if some middleware execute next(new Error(...)), connect will recursive traversal the whole app.stack, until the program meets the error middleware.However, this mechanism may be low-performance.

for example:

var app = connect();
// middleware1
app.use((req,res,next) => {
  next(new Error('some error'));
});
// middleware2
app.use((req,res,next) => {
  next();
});
// middleware3
app.use((err,req,res,next) => {
 next();
});
// middleware4
app.use((req,res,next) => {
  next();
});

in this case, app.stack.length is 4,when in middleware1 executes next(new Error('some error')) , connect will recursive traversal the whole app.stack.

I have an idea

  • the connect error middlewares and no-error middlewares stored in different arrays.
  • app.errware stores error middlewares and app.stack stores no-error middlewares.
  • if any middleware execute next(new Error(...)), program directly invokes error middlewares via app.errware
  • in app.errware array, every object element, contains a index property, which can help connect
    directly skip to the no-error middlewares after invoke error middleware
  • I have forked this project, and Submit the code in a branch.
    https://github.com/vincentLiuxiang/connect/blob/directlyInvokeErrorMiddleware/index.js
function createServer() {
  ...
  app.stack = [];
  // store for error middleware
  app.errware = [];
  return app;
}

proto.use = function use(route, fn) {
  ...
  // error middleware push to this.errware, 
  // and add an  index property equals this.stack.length
  if (handle.length === 4) {
    this.errware.push({ route: path, handle: handle, index:this.stack.length })
  } else {
    this.stack.push({ route: path, handle: handle });
  }

  return this;
};

proto.handle = function handle(req, res, out) {
  ...
  var stack = this.stack;
  var errware = this.errware;
  ...
  function next(err) {
    ...
   // next callback
   // var layer = stack[index++];

   var layer;
   if (err) {
     layer = errware[errIndex++];
     // move stack pointer,skip the no-error middleware
     if (layer) index = layer.index;
   } else {
     layer = stack[index++];
   }

   // all done
   if (!layer) {
     defer(done, err);
     return;
   }
  }
}
@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 9, 2016

Contributor

In order to consider potential performance improvements, please share a test case along with a benchmark showing the before vs after performance.the test should be close to real world connect apps as possible.

Contributor

dougwilson commented Sep 9, 2016

In order to consider potential performance improvements, please share a test case along with a benchmark showing the before vs after performance.the test should be close to real world connect apps as possible.

@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