Skip to content
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

Errors in tests appearing for Node.js >= v10.14.2 #130

Closed
jaydenseric opened this issue Jan 10, 2019 · 2 comments · Fixed by #162
Closed

Errors in tests appearing for Node.js >= v10.14.2 #130

jaydenseric opened this issue Jan 10, 2019 · 2 comments · Fixed by #162

Comments

@jaydenseric
Copy link
Owner

Something that changed between Node.js v10.14.1 and v10.14.2 has caused errors to log in a few places in tests (which still pass):

https://travis-ci.org/jaydenseric/graphql-upload/jobs/477605581#L570

Error: Parse Error
      at socketOnEnd (_http_server.js:455:20)
      at Socket.emit (events.js:193:15)
      at Socket.EventEmitter.emit (domain.js:459:23)
      at endReadableNT (_stream_readable.js:1129:12)
      at process.internalTickCallback (internal/process/next_tick.js:72:19)

We need to work out if its a bug in Node.js, or a graphql-upload compatibility issue with the new Node.js versions.

@jaydenseric
Copy link
Owner Author

These Node.js changes may be related?

  • [cb861a4897] - stream: do not error async iterators on destroy(null) (Matteo Collina) #23901
  • [6f4a638175] - stream: ended streams should resolve the async iteration (Matteo Collina) #23901
  • [ce79ae6544] - stream: async iteration should work with destroyed stream (Matteo Collina) #23785

@mike-marcacci
Copy link
Collaborator

mike-marcacci commented Jan 10, 2019

Just looking at changes in node, this may be related. I think the first thing to do is figure out where these are being logged. I do remember having to wrestle tap at one point to keep it from logging expected errors, so this may be something like an unhandled error event or Promise rejection that tap is helpfully bringing to our attention.

The actual error that is logged is not surprising to see, as we are aborting the request in those tests. I know that between node 8, 9, and 10, errors would appear in the stream's pieces in different orders, so we already have logic that covers a broad range of timings. My initial suspicion is that node made a very subtle change, and we know about (and handle) an error before we would attach an error handler to a downstream piece that previously wasn't propagated the error, but now is.

mike-marcacci added a commit that referenced this issue Oct 7, 2019
I narrowed down these tests to aborting in a Koa app. This happened
because:

1. Koa listens for the error event on the HTTP stream itself, and
   re-emits it on the app's own event emitter.
2. If there is no listener for an app error, Koa will deviate from
   the node default of crashing, and will instead log the error.
3. This particular error originates from within the HTTP stream,
   causing Koa to log the error message.

I fixed this by actually _expecting_ one error to be emit from the
app, adding an assertion to test this.
jaydenseric added a commit that referenced this issue Oct 9, 2019
Enforce 100% code coverage and improve processRequest internals and tests (including a new test using vanilla Node.js HTTP), fixing #130 .
krasivyy3954 added a commit to krasivyy3954/react-graphql-upload that referenced this issue Jan 6, 2023
Enforce 100% code coverage and improve processRequest internals and tests (including a new test using vanilla Node.js HTTP), fixing jaydenseric/graphql-upload#130 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants