Error Handling?

[pyrossh]

There seems to be no error handling. I ran into an error where I had set the files limit to 1 and was trying to upload multiple files and busboy wasn't emitting an event but apollo-upload-server was create a promise that would never resolve and the request would start hanging.

1. We fist need to handle the special limits events in busboy here,
   https://github.com/mscdex/busboy#busboy-special-events
   and reject the promise if any of those are raised.
2. And the inner promise should reject if the mapFieldname does not match the fieldName,

if (fieldName === mapFieldname) { // }
else { reject(new Error(`Field name mismatches ${fieldName} !== ${mapFieldname}`));

3. I guess we should resolve the top level promise only on the busboy finish event.

And finally maybe the request.pipe/busboy might have some error events which need to consumed and reject the promise for safety sake.

[pyrossh]

Also we can use pump to handle pipe errors and make sure the pipe is finished.
https://github.com/mafintosh/pump

[jaydenseric]

I thought I had tested the limits out when I wrote it; it will take me a bit to recall the inner workings again.

On point 1...

I wonder if busboy throws a general error event at the same time as the "special" filesLimit, etc. events.

If it does, perhaps this change is all we need:

// Upload scalar
const upload = new Promise((resolve, reject) => {
  busboy.on(
    'file',
    (fieldName, stream, filename, encoding, mimetype) =>
      fieldName === mapFieldName &&
      resolve({ stream, filename, mimetype, encoding })
  )

  busboy.on('error', reject)
})

Otherwise we would need to listen for the relevant individual "special" events to reject the promise.

I'm not following on point 2. The fieldName and mapFieldName will purposefully not match several times until only the right file is encountered.

Very edge case, but if a malformed request has more files in the map than are attached to the form, the upload scalar promise will never resolve. We could guard against that adding something like this:

busboy.on('finish', () => reject(new Error('File missing from the multipart request')))

Keep in mind that we shouldn't block the individual Upload scalar promises from resolving by awaiting for the whole form parse.

I'm not sure if pump is necessary. What specific issue would it fix?

[dizlexik]

I just ran into this problem too. I have a maxFileSize of 25mb set and tried to upload a 100mb file. No error was thrown and I ended up with a corrupt 25mb file on my server. I expected this to error out.

[jaydenseric]

@dizlexik that might be working as expected; it is up to you to handle that case on the file stream according to busboy behavior:

If a configured file size limit was reached, stream will both have a boolean property truncated (best checked at the end of the stream) and emit a limit event to notify you when this happens.

[dizlexik]

I actually did dig into this earlier and saw what you're referencing. That behavior might make sense for Busboy, but it seems to me that the limit event should be handled by apollo-upload-server and an error thrown. I feel like maxFileSize implies that an error will be thrown when it is exceeded and I can't imagine that I'm alone in that. What do you think?

[jaydenseric]

At the very least we should add readme content explaining how the various errors are caught.

We might also want to account for that type of error in the example resolvers.

The tricky thing with maxFileSize is that it is a stream error that happens after the Upload scalar promise has already resolved.

[dizlexik]

I understand that this cannot be caught within the Upload promise.

As far as the behavior of the stream, though, it seems to me that the limit event is an implementation detail of using Busboy that should not be imposed upon the consumer of apollo-upload-server. Would it make sense to expose a transformed stream that emits an error when the limit event is encountered? I think that would be the expected behavior of most users.

[jaydenseric]

I have been working on error handling all day, and have made a lot of progress but it might take a bit longer to work out the kinks. It's amazing how little info there is on handling edge cases with Node.js streams and multipart requests, such as handling users aborting the request halfway though. Even basic questions, like how to deliberately interrupt and shutdown a stream with an error are hard to Google.

In the end, File upload scalar promises will reject with meaningful errors for all the various cases, and if an error such as a file size limit or aborted request happens when a file is still being streamed the stream will receive a meaningful error event. If a request is aborted, cutting off a stream, the stream will have a truncated property like what busboy does for files that are too big.

Really complicated 😓

[pyrossh]

True True. I remember struggling with req/res stream errors 2 years ago. We used domains and try catches like crazy then to handle all possible edge cases and still could not fully stop the server from crashing. Though when we moved onto golang it was a real breeze and never had issues with these sort of errors albeit a little verbose but a little less stress.

[dorshinar]

Any news about it? The issue is stale for over a year and it has not been resolved, nor properly documented.

[jaydenseric]

@dorshinar this issue was resolved; if you're experiencing a bug please open a new issue with some minimal reproduction steps and we can take a look.

[aarabmed]

i did a console log for the fileInput and it seems to be an array of promises, so as an error handling for maxFiles im checking the length of that array for a specific value, is this a good way to do it ?

[SergioSuarezDev]

I just ran into this problem too. I have a maxFileSize of 25mb set and tried to upload a 100mb file. No error was thrown and I ended up with a corrupt 25mb file on my server. I expected this to error out.

I have same problem and I created a issue: #274

[forrestwilkins]

@jaydenseric I'm still facing this same issue. Are we sure this was fully resolved?