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

Should handle unexpected streaming errors #77

Closed
koresar opened this issue May 17, 2018 · 4 comments · Fixed by #81
Closed

Should handle unexpected streaming errors #77

koresar opened this issue May 17, 2018 · 4 comments · Fixed by #81

Comments

@koresar
Copy link

koresar commented May 17, 2018

The module does not handle unexpected errors happened in the middle of the file streaming. This happens ~4% of the times on 3G networks (last time I checked).

A good example on how to handle them can be found here: https://github.com/expressjs/multer/blob/80ee2f52432cc0c81c93b03c6b0b448af1f626e5/lib/make-middleware.js#L134

        parser.on('error', function (err) {
          pendingWrites.decrement()
          abortWithError(err)
        })

Currently, it looks like the module crashes the node.js process. (I do not have proofs of this though.)

@jaydenseric
Copy link
Owner

jaydenseric commented May 17, 2018

I have found it very challenging to verify uploads aborted by the client work ok. Some time ago I spent the better part of a week trying to workout a way to reliably test a disconnection and decided it was too difficult. If you or anyone else could work out a way to do so it would be a really big help!

Client aborted uploads have been manually tested to fail gracefully (using apollo-upload-examples, large files and network throttling in Chrome dev tools), although I can't rule out something has changed or was missed.

Errors must be listened for on every upload file stream, or they will be uncaught and crash your server. Are you sure that is not the case?

@koresar
Copy link
Author

koresar commented May 18, 2018

I'll see what I can do. Seems doable by mocking the request object passed to the processRequest() function.

@jaydenseric
Copy link
Owner

Here is an issue for adding tests for client aborted requests: #79.

@jaydenseric
Copy link
Owner

An easy way to cause a crash is to do an upload mutation with a GraphQL error in the query (ask for a field that does not exist or something). The GraphQL server rejects the request as soon as it validates the query, and disconnects before all the uploads resolve causing FileStreamDisconnectUploadError: Request disconnected during file upload stream parsing..

At least thats what happens with graphql-api-koa (I don't use Apollo server anymore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants