Upload being cancelled crashed the Node process

elwinar commented 2 years ago

I'm implementing an upload form for large files, and everything is dandy except for one particular issue: when the user closes its browser (which cancels the request), the node process crashes.

I've been able to trace the issue to this line: https://github.com/jaydenseric/graphql-upload/blob/master/processRequest.js#L335, and confirmed it by commenting out the code.

Is there a way to prevent the crash I didn't see?

elwinar commented 2 years ago

Here is the stack trace I receive.

events.js:377
      throw er; // Unhandled 'error' event
      ^
BadRequestError: Request disconnected during file upload stream parsing.
    at IncomingMessage.<anonymous> (/root/graphql-video.js:63386:16)
    at Object.onceWrapper (events.js:519:28)
    at IncomingMessage.emit (events.js:400:28)
    at IncomingMessage.emit (domain.js:475:12)
    at abortIncoming (_http_server.js:570:9)
    at socketOnClose (_http_server.js:562:3)
    at Socket.emit (events.js:412:35)
    at Socket.emit (domain.js:475:12)
    at TCP.<anonymous> (net.js:686:12)
Emitted 'error' event on FormData2 instance at:
    at FormData2.onerror (internal/streams/legacy.js:62:12)
    at FormData2.emit (events.js:400:28)
    at FormData2.emit (domain.js:475:12)
    at FormData2.CombinedStream._emitError (/root/graphql-video.js:63686:12)
    at DelayedStream.<anonymous> (/root/graphql-video.js:63626:15)
    at DelayedStream.emit (events.js:412:35)
    at DelayedStream.emit (domain.js:475:12)
    at DelayedStream._handleEmit (/root/graphql-video.js:63497:19)
    at ReadStream.source.emit (/root/graphql-video.js:63454:23)
    at emitErrorNT (internal/streams/destroy.js:106:8) {
  expose: true,
  statusCode: 499,
  status: 499
}

jaydenseric commented 2 years ago

Are you sure you are using an up-to-date graphql-upload version, and you are handling errors when you are promisifying the individual file upload streams?

We have tests for client aborted requests:

Note that if the client aborts before the second chunk of a file's stream has been sent, createReadStream() will throw, so make sure you're wrapping that call in a try catch as well as listening for the error events.

If you are doing all these things correctly, we can dig deeper and see if there is a graphql-upload bug lurking.

elwinar commented 2 years ago

Hey. I haven't had much time to work on this these days, but I'll try to check everything today.

I'm pretty sure that I've tried adding promises and try-catches around the whole resolver, I'll have another go at it and try to make a minimal testable example. The issue being that the files need to be very large to test in local (I'm testing through a VPN).

elwinar commented 2 years ago

OK, I've found the issue working by dichotomy.

It was on the other side of the resolver: my graphql server is forwarding the multipart request to a backend API, and it was the multipart call towards the backend API that was failing. I have no clue why this is failing there, why adding an error event on the stream itself doesn't catch the error, etc. Between the promises, error events, and exceptions, I must admit I'm a little lost with proper error handling and propagation in Node.

Thanks for taking the time to read my ignorant issue :D .

jaydenseric / graphql-upload

Upload being cancelled crashed the Node process #322