fix: manually track if request is ended or not

[SimenB]

When upgrading from node 14 to node 16, we started getting Request disconnected during file upload stream parsing. errors. Putting some log statements inside the end and close event listeners shows that they are called in the correct order

Using a boolean and manually tracking solves the issue.

I've tried to figure out if there were timing changes in the events in Node 16 without luck. I also don't know how to write a test for this...

---

For now I just use a patch to apply the change from this PR to our project. I don't really have the time to write a test, but I thought to open a PR in case it can be a start for a proper fix 🙂

[jaydenseric]

Thanks for bringing this up.

What scenarios are you noticing this issue arise for? A correct GraphQL multipart request (e.g. single file upload) that if not for this bug, should have succeeded?

Node.js v16 is tested quite thoroughly against most possible scenarios in GitHub Actions CI, and this particular issue hasn't come up yet. What specific versions of Node.js v16 are you noticing the issue with? I wonder if it's also buggy in Node.js v17, which is not tested yet in CI but all the tests pass locally for me.

I also don't know how to write a test for this...

Yea, I'm not sure either. Node.js streams timing issues can be tricky to figure out and reproduce sometimes.

For this project we try get to the bottom of any mysterious issues before figuring out the most elegant solution, otherwise we might miss other locations in the code that have similar problems, and also with so many streams and events and things going on the code becomes a tangled mess pretty quick.

I wasn't planning to work on graphql-upload this week but I'll see what I can do :)

[SimenB]

What scenarios are you noticing this issue arise for? A correct GraphQL multipart request (e.g. single file upload) that if not for this bug, should have succeeded?

Correct - it works correctly in Node 14 (I've never seen the processing error except in the correct cases where the request is cut off), but when upgrading to 16 (now that it's LTS) threw this error for every single upload, without ever being successful. Zero changes to any code in the client or on the server, just a pure node 14 -> node 16. And since manually tracking works, it points to some sort of nextTick somewhere being added or removed (in how the events are emitted or in how/when removeListener runs).

[SimenB]

I did a tiny bit more digging now - node 16.0.0 fails, while the last v15 release (15.14.0) works. Nothing from https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V16.md#2021-04-20-version-1600-current-bethgriggs jumps out at me

[jaydenseric]

I've been working on some updates the past few days, and have discovered 2 potential bugs (unrelated to this issue/PR). Gross Node.js streams stuff.

Here is a suggestion as an alternative to this PR:

https://github.com/jaydenseric/graphql-upload/blob/6158486a5580788a798961d3db1af945398ddbcd/public/processRequest.js#L354-L357

-     request.once('close', abort);
+     request.once('close', () => {
+       if (!request.readableEnded) abort();
+     });
-     request.once('end', () => {
-       request.removeListener('close', abort);
-     });

Because I can't reproduce the issue you've been experiencing, perhaps you could try that out and see if it works?

I haven't used readable.readableEnded before but it seems appropriate in this situation, and it fits our current level of Node.js version support:

https://nodejs.org/api/stream.html#readablereadableended

[SimenB]

@jaydenseric thanks! Can confirm v13 works for us 🙂