The middleware pipeline should halt on error #1042

Open
dougwilson opened this Issue Jun 9, 2014 · 6 comments

Comments

Projects
None yet
2 participants
@dougwilson
Contributor

dougwilson commented Jun 9, 2014

Right now, when next(err) is called, the pipeline will switch into an "error mode" and start to process the error down the error middleware. If some middleware then calls next() (i.e. double-next) then the normal pipeline processing will resume.

I think that once an err has been provided to the pipeline, a flag should be set in the pipeline and simply ignore all next() calls (non-error-pipeline calls). This would probably be a sane way to manage errors, as it would be an analogue to someone raising a flag when they get on('error', fn) called and want to ignore the results from non-cancel-able async operations.

@dougwilson dougwilson added the Idea label Jun 9, 2014

@dougwilson dougwilson self-assigned this Jun 9, 2014

@dougwilson dougwilson changed the title from The middleware pipe line should halt on error to The middleware pipeline should halt on error Jun 9, 2014

@dougwilson dougwilson modified the milestone: 4.0.0 Feb 1, 2015

@TrejGun

This comment has been minimized.

Show comment Hide comment
@TrejGun

TrejGun Aug 4, 2017

i'm not agree with you. my argument is a then-catch flow from promises

new Promise(resolve => {
	setImmediate(() => {
		resolve();
	});
})
	.catch(() => {
		console.log("will NOT come here");
	})
	.then(() => {
		console.log("will come here 1");
		throw new Error();
	})
	.then(() => {
		console.log("will NOT come here");
	})
	.catch(e => {
		console.log("will come here 2");
		throw new e;
	})
	.catch(e => {
		console.log("will come here 3");
	})
	.catch(() => {
		console.log("will NOT come here");
	})
	.then(() => {
		console.log("will come here 4");
	});

TrejGun commented Aug 4, 2017

i'm not agree with you. my argument is a then-catch flow from promises

new Promise(resolve => {
	setImmediate(() => {
		resolve();
	});
})
	.catch(() => {
		console.log("will NOT come here");
	})
	.then(() => {
		console.log("will come here 1");
		throw new Error();
	})
	.then(() => {
		console.log("will NOT come here");
	})
	.catch(e => {
		console.log("will come here 2");
		throw new e;
	})
	.catch(e => {
		console.log("will come here 3");
	})
	.catch(() => {
		console.log("will NOT come here");
	})
	.then(() => {
		console.log("will come here 4");
	});
@TrejGun

This comment has been minimized.

Show comment Hide comment
@TrejGun

TrejGun Aug 4, 2017

I mean if you ever finish express5 which is going to support promises, this is what people will expect from its behavior

TrejGun commented Aug 4, 2017

I mean if you ever finish express5 which is going to support promises, this is what people will expect from its behavior

@dougwilson

This comment has been minimized.

Show comment Hide comment
@dougwilson

dougwilson Aug 5, 2017

Contributor

Hi @TrejGun good point. What is your idea for how to change the behavior to solve the double-next() issues?

Contributor

dougwilson commented Aug 5, 2017

Hi @TrejGun good point. What is your idea for how to change the behavior to solve the double-next() issues?

@TrejGun

This comment has been minimized.

Show comment Hide comment
@TrejGun

TrejGun Aug 6, 2017

i believe when you want to pass an error between error-middlewares you should call next(error) but not next()

app.use(function (err, req, res, next) {
  next(err);
})

if you call next() from error middleware

app.use(function (err, req, res, next) {
  next();
})

you should resume non-error-pipeline

TrejGun commented Aug 6, 2017

i believe when you want to pass an error between error-middlewares you should call next(error) but not next()

app.use(function (err, req, res, next) {
  next(err);
})

if you call next() from error middleware

app.use(function (err, req, res, next) {
  next();
})

you should resume non-error-pipeline

@dougwilson

This comment has been minimized.

Show comment Hide comment
@dougwilson

dougwilson Aug 8, 2017

Contributor

Gotcha. So what I'm trying to brain storm is a solution to the following problem:

app.use(function (req, res, next) {
  setImmediate(function () {
    // some async activity fails
    next(new Error())
  })
  setImmediate(function () {
    // user has logic error that didn't stop the processing
    // so a second next() occurs thinking successful
    next()
  })
})

So the issue I'm trying to think of how to handle better is that second next() call, after an error occurs. Right now it breaks really odd, and of course is all silent about how that happens. It basically will re-enter into the middleware processing even after the res may have been written to because of the error handling, resulting in those annoying "can't send headers" errors at best, silent race conditions at worse.

What is your take on a solution? How do Promises handle the situation?

Contributor

dougwilson commented Aug 8, 2017

Gotcha. So what I'm trying to brain storm is a solution to the following problem:

app.use(function (req, res, next) {
  setImmediate(function () {
    // some async activity fails
    next(new Error())
  })
  setImmediate(function () {
    // user has logic error that didn't stop the processing
    // so a second next() occurs thinking successful
    next()
  })
})

So the issue I'm trying to think of how to handle better is that second next() call, after an error occurs. Right now it breaks really odd, and of course is all silent about how that happens. It basically will re-enter into the middleware processing even after the res may have been written to because of the error handling, resulting in those annoying "can't send headers" errors at best, silent race conditions at worse.

What is your take on a solution? How do Promises handle the situation?

@dougwilson dougwilson removed this from the 4.0.0 milestone Aug 8, 2017

@TrejGun

This comment has been minimized.

Show comment Hide comment
@TrejGun

TrejGun Aug 8, 2017

new Promise((resolve, reject) => {
	setImmediate(() => {
		reject(new Error());
	});
	setImmediate(() => {
		resolve();
	});
})
	.then(() => {
		console.log("then");
	})
	.catch(e => {
		console.log("catch", e);
	});

says

catch Error
    at Immediate.setImmediate (~/test.js:3:10)
    at runCallback (timers.js:781:20)
    at tryOnImmediate (timers.js:743:5)
    at processImmediate [as _immediateCallback] (timers.js:714:5)

so just skip second call completely but put a warning to console

Callback called more than once.

TrejGun commented Aug 8, 2017

new Promise((resolve, reject) => {
	setImmediate(() => {
		reject(new Error());
	});
	setImmediate(() => {
		resolve();
	});
})
	.then(() => {
		console.log("then");
	})
	.catch(e => {
		console.log("catch", e);
	});

says

catch Error
    at Immediate.setImmediate (~/test.js:3:10)
    at runCallback (timers.js:781:20)
    at tryOnImmediate (timers.js:743:5)
    at processImmediate [as _immediateCallback] (timers.js:714:5)

so just skip second call completely but put a warning to console

Callback called more than once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment