Proxy res.end(callback) calls correctly

[theneva]

Hi! :wave:

This fixes #477. It bit me, and figuring out what broke in my application took forever, so I figured I'd try to fix it.

I'm not familiar with this code base at all, so please excuse any style breaches!

The test I added fails without the changes to index.js, and all the tests pass with the changes.

I will also attempt sending a PR updating the express docs on res.end([data] [, encoding]) at https://expressjs.com/en/api.html#res, as they seem to suggest that res.end does not take a callback. It does!

Let me know if anything looks off!

[theneva]

Ergh, I see the build failed on Node v0.8.28 and v0.10.48 with precisely the error I was trying to patch away. I have no idea why… Does this project actively support those long-EOL-ed releases?

[ghinks]

looking into build failure ... it appears typeof support was not available in node 0.10 (yes I know it is old )

[theneva]

Hm, I guess I only solved my own case here :smile: I'll have another look at this tomorrow! Thanks for taking a look!

And thank you very much @ghinks, it would be super helpful if you found something! It doesn't look like the res#end callback arguments have changed at all between pre-0.8 and latest.

Edit: Oh, typeof doesn't work… I'll work around that, then 🙂 Edit 2: https://stackoverflow.com/a/7356528 seems like a decent way to check if it's a function, so I guess I'll use that unless anyone objects

[ghinks]

yes, I'm surprised by the fail too. I thought this was part of the language.

[theneva]

Had another look!

typeof is not the offender here – it is supported in both Node 0.8 and 0.10.

The builds are failing because Response#end's callback argument was only added in Node 0.11.6 (https://nodejs.org/en/blog/release/v0.11.6/). This change is not noted in the "History" block in the Node docs. I'll send them a PR.

I suppose this is why the main Express docs on Response#end don't mention the callback. The Express docs state that:

This method actually comes from Node core, specifically the response.end() method of http.ServerResponse.

… and link to the latest Node (14.2.0) docs. This led me to believe there was an error in the Express docs, as I noted in the original post.

Versions prior to 0.11.6 (including both 0.8 and 0.10, obviously) simply check if data is truthy, and call Response#write(data, encoding). That doesn't work when data is a function, which is what happened in the tests.

express-session could support passing the callback on if the runtime supports it, and warn about usage if it doesn't. That way it would support my use case without dropping support for 0.8 and 0.10. I'm thinking:

1. Check if Response#end supports the callback at runtime (res.end.length > 2 to verify that it takes at least 3 arguments)
2. If it doesn't, log a warning and keep the current behaviour
3. If it does, support the arguments in exactly the same way as Response#end does today

If you're okay with this approach, I'll try to make it work regardless of whether you modify the session, and the various supported permutations of arguments.

What do you think?

[dougwilson]

Hi @theneva thanks for taking a deep look! All of that sounds good, except about the warnings part. I would say the goal I think would be to have res.end behave the same with or without this module loaded. That means that unless those version of Node.js are issuing warnings when the callback is supplied, this module shouldn't be, either.

[theneva]

Alright, sounds good! I'll take another stab at it tomorrow, then.

[dougwilson]

Oh, forgot to comment on the res.end.length part as well: that may not be the best method, as it can be common for AOP-style things to have a length of zero, and just return orig.apply(this, arguments) which keeps the original behavior intact, but the arguments length is no longer "original". It may be enough to just always pass it regardless, and let the original implement do with it as it would otherwise if it had been passed when it didn't understand.

[jonchurch]

Linking for context, here's a PR tackling very nearly the same issue over in compression https://github.com/expressjs/compression/pull/101

[theneva]

Thanks, @jonchurch! I think I'm pretty close to coming up from the rabbit hole that is undocumented http module behaviour… Perhaps we can learn from each other when I've reached a point that works – hopefully soon :smile:

To anyone subscribed to e-mails about new commits being pushed, sorry! I've been fiddling with this on and off using two different computers, and I guess there's no nice way to avoid notifying everyone. I'll convert the PR to a draft now in case that helps.

I think I'm very close something that works & can be reviewed/discussed now.

[theneva]

@dougwilson I think I finally have something that works as intended now, with tests for all cases.

As it turns out, there's a lot of undocumented and/or weird behaviour in old versions of Node.

The coverage check fails because of some error handling I had to introduce to write tests. I'm not too happy about how that turned out, so I'd very much like to know if you can think of a better way to do the error handling before I spend more time trying to get it covered by the tests.

There is a very minor difference between the original res#end and the proxy, in that calling res.end('non-empty string', callback) will not invoke the callback immediately after write in the proxy. I believe this behaviour is a bug in the early versions of Node, and I've written comments where it applies.

I'm also not too happy about all the code duplication in the tests, but I couldn't find an elegant way to test multiple cases at once.

It would be awesome if you took another look at the code!

[dougwilson]

Hi @theneva I just wanted to write in here that I haven't forgotten about this PR; it is just quite complicated, and really appreciate all the work you did on it :) I am just trying to wrap my head around some of the items in here where you have questions in order to get you some answers.

[theneva]

Heh, yeah, it turned into a lot. For what it's worth, I've worked around the issue, so it's not hugely important to me that this lands. It's wrong, but maybe the added complexity isn't worth it...

[dougwilson]

At some level it is worth fixing, but I know we have gotten away with not fixing it just because I guess the feature is rarely used. But of course this still breaks the callback feature, so should be fixed.

[theneva]

Yeah. I think the worst part for me was how hard this was to debug and pinpoint the issue, but fixing it would obviously be best :)