Prevent next from being called multiple times

[calebmer]

An example of a usage for this reflection property would be in layer code:

function isLayerStillWaiting(next, layer) { return next._currentLayer === layer }

This would be useful as router tries to support different async language constructs. Specifically (in the case of #25) Promises.

Some issues to address off the top of my head:

• Only accounts for the present next state
• next should probably be associated with its layer so it can be locked down once used

[calebmer]

Any ideas for solutions would be great, I'll take a stab at it when I have some time.

I didn't understand the depth of this till just now...

[dougwilson]

I'm not sure I understand the purpose of this? Adding a property on next has two repercussions:

1. People will now be able to inspect it for hacky things and then we'll have to keep the property intact for a long time.
2. People will alter this property for their own weird purposes, which can have unintended effects.

From what I can tell, the purpose of this PR is to expose a property on the next function instance. This raises the following questions to be:

1. Who will be looking at this value?
2. Should the value be updated to null or something when we call done, or will the _currentLayer property just be left hanging at the last layer?
3. router.use also implements a next() mechanism. Does this also need to expose this property?

[calebmer]

I agree the property method might not be the best idea. To my knowledge there is nothing stopping the user from doing this:

next()
next()

In an async context next could be called after reject, so a user might be confused and write:

reject(new Error('sad'))
next()

Which calls next twice like above.

I propose instead of using a property (with all the faults you mention) instead we wrap next in a once function at the layer level here, or here as you brought up. I haven't looked too much into these parts of the code so I would need your recommendation.

[calebmer]

Ok, removed the next property in favor of using a once wrapper.

This error is uncatchable as it would require yet another next call (inception bwah). It is to my knowledge that calling next more than once in the same middleware is an anti-pattern so this doesn't worry me as the error should rarely ever appear and even so it would be a developer warning.

The minor code duplication in handle_request and handle_error will be fixed with the #25 addition of the handle_any method.

[dougwilson]

Only calling next() more than once is an anti-pattern; it is completely acceptable to call next(err) as many times as you like, even after you already called next().

[dougwilson]

We definitely do not want to ever purposely throw something that is un-catachable in some way.

[calebmer]

Ok, made the change so that errors would be catchable. However, it is most likely that the response will be sent before the error handlers are triggered, so res.end will likely also be called twice. If calling next(err) is not an anti pattern however, this should not be an issue.

[calebmer]

Now calling next(err) multiple times is okay and it will behave as normal.

Edit: but tests are failing...one sec

[calebmer]

Tests are passing again on all versions. Node v0.8.x does not have a res.headersSent property, this broke tests.

[dougwilson]

Just use if (res._header) for the condition for full Node.js compatibility.

Even then, this kind of a change in a fundamental utility is not good and we don't want; it would hide errors silently.

[calebmer]

Ok, made it a config option.

[calebmer]

What's the status on this PR? Once it is merged I can refactor #25 and get that ready for merger.

[calebmer]

Poke

[calebmer]

@dougwilson This pr is blocking future development, what's the status?

[dougwilson]

Hi, sorry I was away on some holiday. It all seems fine, but can you show though a benchmark what the performance change is between making a new function object for every single middleware call vs the old way?

Also, sorry if this is hurting you, but you can always use npm to depend on a specific commit in a git repo, so you don't need something published to npm to use it (and besides, this is just something to help developers from foot guns, not a critical bug fix). We are all volunteers and are unpaid, and all of this is in our free time, which can vary a lot, I'm sorry.

[dougwilson]

As a reminder to myself, I need to setup a v2 branch to merge this change into as well.

[dougwilson]

So I was looking through the current change set here and there are a few minor issue. The GitHub site on my phone does not make it easy to add new line comments, so I will get them added for you when I get to a computer.

[calebmer]

I apologize for interrupting your vacation, take all the time you need, your dedication to the open source community is very much appreciated. It was the lack of communication that was scary.

Here is a benchmark, results are posted in a comment. At about 1600 middleware functions the current version starts throwing stack size exceeded errors and with this change that number is decreased to about 1500 middleware (middleware being a super simple next() call). As for time, the new method adds about 1-2ms as we get into the 900+ middleware range.

[felixfbecker]

The stack overflow will probably not occur in real apps because most of the time you fetch something async so stack trace is cleaned...
@dougwilson any update on this?

[calebmer]

@felixfbecker still waiting on feedback from @dougwilson 😊

[felixfbecker]

It's been a while...

[dougwilson]

@felixfbecker, I know it has, but I just truly have not had the time to focus on taking a look, though this is a wanted feature. There is an issue in the main Express repository that hopefully helps shed light on this and I am truly sorry of the time it has taken.

As I have put all support for Express on hold while I work with IBM, I have had time to address various long-standing issues over this weekend, which has been nice. Because there has been so much sitting around, I'm trying to at least dig through them in a fair order, which is FIFO.

[felixfbecker]

@dougwilson you are doing great work. I just wanted to know the status :)

[tunniclm]

@pillarjs/express-tc Release 5.0 item "Add support for Promises in all handlers expressjs/express#2259" is being developed in #25 which is waiting for this PR (#26). I just wanted to flag this up, since it's a few levels deep.

[calebmer]

@tunniclm an alternative of the promise implementation is by @blakeembrey in #32. In my implementation I decided to also fix this error which may be common in a promise environment.

As another note, I have been been using the promise implementation in #25 (specifically the master branch of my fork) in production for a while now with a beta version of Express 5.

[wesleytodd]

I believe that this PR is both out of date enough and also not clear on the goals that we cannot land it as is. My guess is that we should re-try anything like this as a new pr and not an update to this one. Because of that I am going to close it. If we want to address this I would say that we start with a discussion of goals in a new issue, talk about the plan, and only then open a new pr.