FileStreamDisconnectUploadError when checking file validity

[FreddyPoly]

Hello,

The problem

I have a problem when checking for the validity of the files sent by multipart request from the client side.

The client sends a multipart request with maximum 5 different files, then the server loops through them to check if every file is an instance of stream.Readable. While looping through the files an error occures: FileStreamDisconnectUploadError: Request disconnected during file upload stream parsing..

The first file is checked but then the others files are never read.

This is what my loop looks like:

for (const pic of data.pictures) {
      const resolved = await pic;
      if (!(resolved.stream instanceof stream.Readable)) return new Error('Wrong file');
}

The error is raised when executing the line const resolved = await pic;.
The first file fills the resolved variable but it never gets filled again with the next files.

What I tried

• I tried to destroy the stream after the check: resolved.stream.destroy();
• I tried to replace the for...of loop with a map

Some thoughts

Before making the requests by the client, I sent the requests using curl according to these examples. But the error still occured.

I work on a Macbook with MacOS Sierra 10.12.6 and curl 7.54.0 which uses SecureTransport, as it is the native TLS library.

What is interesting is that my coworkers, which work with the same Macbook as mine but with MacOS High Sierra and curl 7.54.0 which uses LibreSSL as TLS library, manage to get the requests work with the server. Of course we tried with the exact same requests and on a server running on my mac.

Do you have any idea why the error occures and why does the await pic never manage to complete ?

Thanks for your time.

[koresar]

@jaydenseric the apollo-upload-server module crashes the whole node.js process if error happens somewhere in the middle. But my code have all the error handlers possible.

events.js:183
      throw er; // Unhandled 'error' event
      ^

FileStreamDisconnectUploadError: Request disconnected during file upload stream parsing.
    at IncomingMessage.request.on (/Users/koresar/code/my-project/node_modules/apollo-upload-server/lib/middleware.js:151:15)
    at emitNone (events.js:106:13)
    ...SNIP node.js internals...

My code is exactly the same as @FreddyPoly, but without TLS (plain localhost HTTP).

However, that happens occasionally. Could not find stable scenario. Sorry.

[jaydenseric]

I think you will run into problems when attempting to await in a for loop.

It is better to use Promise.all, or the promises-all approach used in the example API.

[koresar]

@jaydenseric I kind of thought it's vice versa. :) I should await sequentially.

[jaydenseric]

@koresar I'm just thinking out loud, and suggestions could be wrong. Node.js streams are a trial and error nightmare.

@FreddyPoly I'm not quite understanding why you are doing instanceof stream.Readable. Shouldn't all file upload streams should be of the same type? If the first file fails that check, then does the new Error cause the loop, and resolver to bomb? Because if you throw an error and send and early response you would expect all the promises of the remaining uploads to reject with FileStreamDisconnectUploadError. You could catch them all, and ignore them if you can somehow signal to the handlers that they were deliberately caused errors.

[FreddyPoly]

I'm sorry I don't quite get your point of view when you say:

You could catch them all, and ignore them if you can somehow signal to the handlers that they were deliberately caused errors.

• Do you mean catching the errors I throw while looping through the files ? So it doesn't reject all the promises ?
• And shouldn't I check for the type of file ? Couldn't I get a file which is not a valid stream.readable ? I checked for "security" reasons.

I'm sorry to bother you with what must be a misunderstanding from my part, this is really a great library you made and I'm glad I get to work with it. GraphQL is so easy to work with and adding a system to get files upload is what makes it completely usable.

What I try to do is check if all the files I get from the client are indeed stream.Readable and if every file is an image, before doing anything with them.

In my resolver, my first action is to call a validator service to check the files:

Mutation: {
    add: async (_, data) => {
      const error = await validationService.filesValidator(data);
      if (error) throw error;

      const offer = await Offer.add(data);

      return offer;
    },
}

If the files are not valid I expect my server to throw an error according to the type of error.

My validation service is where I had my for...of loop running. Now I try to replace it with Promise.all and a map as you suggested and as it is stated in the docs.

async processFile(file) {
    const resolved = await file;

    if (!(resolved.stream instanceof stream.Readable)) return new Error('Wrong file');
    if (!(/\.(gif|jpg|jpeg|tiff|png)$/i).test(resolved.filename)) return new Error('The picture must be a valid file (gif, jpg, jpeg, tiff, png)');

    return null;
  },
async filesValidator(data) {
    if (!data.pictures) return new Error('An offer needs at least 1 picture to be created');
    if (data.pictures.length > 5) return new Error('A maximum of 5 pictures per offer is allowed');

    await Promise.all(data.pictures.map((pic) => {
      this.processFile(pic.file);

      return pic;
    }));
    // gets executed before the end of the map loop

    return null;
  },

The code above "works" but the loop in the Promise.all doesn't finish before executing the rest of the code. If I write the loop differently await Promise.all(data.pictures.map(this.processFile)); I get the error I had formerly.

I also tried with promises-all but it didn't work either

async processFile(pic) {
    const resolved = await pic.file;

    if (!(resolved.stream instanceof stream.Readable)) return new Error('Wrong file');
    if (!(/\.(gif|jpg|jpeg|tiff|png)$/i).test(resolved.filename)) return new Error('The picture must be a valid file (gif, jpg, jpeg, tiff, png)');

    return null;
  },
  async filesValidator(data) {
    if (!data.pictures) return new Error('An offer needs at least 1 picture to be created');
    if (data.pictures.length > 5) return new Error('A maximum of 5 pictures per offer is allowed');

    await promisesAll.all(data.pictures.map(this.processFile));

    return null;
  },

I'm currently trying to change my implementation but I don't get a stable version. I'll post my solution if I get it to work.

Thanks again for your help and your responses.

[FreddyPoly]

Ok it definitely looks like my Macbook has some configuration that makes the request fail. My coworkers who have the same OS version as mine don't manage either to request the server without the error.

So those who are on MacOS High Sierra doesn't have any problem and those who are on MacOS Sierra can't get the request to succeed.

• Maybe the multipart requests aren't build the same way depending on the OS ?
• The file streams aren't handled the same way depending on the OS ?

@jaydenseric Thanks again for your time and your help but I don't think my problem comes from your library or the implementation of the resolver. I guess the OS or some configuration is the key here, pretty hard to know exactly what is the cause. Do you have any idea on what could be different between the two ?

@koresar Did you manage to solve the error ? I would be curious to know how if that's the case and if you don't mind to share your code :)

[jaydenseric]

@FreddyPoly Although the OS may be the same, what about the hardware? Is one SSD, more RAM, or better CPU? Maybe it is a race condition somewhere.

[koresar]

I couldn't solve it unfortunately. It's not reproducible when I try. Sad thing it happened in production, but only once. It all began after I introduced the for-of loop with await in it. Can it be a missing "error" event handler of the incoming readable stream? Especially when there are multiple files in the request.

…

On Sat., 7 Apr. 2018, 09:06 Jayden Seric, ***@***.***> wrote: @FreddyPoly <https://github.com/FreddyPoly> Although the OS may be the same, what about the hardware? Is one SSD, more RAM, or better CPU? Maybe it is a race condition somewhere. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABjCL_nc64u7Meijs8tXWZyxEF43vNaFks5tl_UBgaJpZM4TGlhB> .

[FreddyPoly]

@jaydenseric We changed computers recently and we all have the exact same Macbook, so this mustn't be a hardware problem.

I'll investigate some more to find what's the cause of the error.

[hkjeffchan]

SELF EDIT: this comment is WRONG, you may just skip to #65 (comment)

I ran into similar problem.
Solution: Create a new promise for your operation on the stream and then await Promise.all for your new promises. Do NOT await Promise.all for the streams to resolve and then do your operation with map. The first stream will block all other streams to resolve.

This is similar to RXJS switchMap and then combineAll

[koresar]

Trying to understand your suggestion @hkjeffchan. If I rephrase your advice would it sound right?

If a stream fails you should break the loop and NOT process any further file streams.

[FreddyPoly]

I don't get your solution @hkjeffchan . Could you provide an example please ? I can't get this to work.

As I said I investigated to see what was happening and indeed the second file never gets resolved. The first file gets resolved without any problem but the busboy's parser never executes the on file event for the next files.

And this happens only if in the resolver, I await Promise.all the map to loop through the files I received.

[hkjeffchan]

Sorry, it was 5AM in my timezone yesterday when I wrote. I try to explain more in details.

for (const pic of data.pictures) {
      const resolved = await pic;
      if (!(resolved.stream instanceof stream.Readable)) return new Error('Wrong file');
}

The stream return by "await pic" is a nodejs readable stream and it is a series of data chuck. You may think of it like:
stream = [data1, data2, data3 ... data100]

After you do "const resolved = await pic;", a data chuck will be waiting for you to process. If you don't process, the busboy parser will stuck. When it timeouts, it will raise the FileStreamDisconnectUploadError. You need to CONSUME the data chuck so that busboy parser will continue to process the next data chuck (may or maynot be the same stream). If you need to write to a new file, just pipe to a writable stream. Even if you throw an error, you still need to CONSUME the data chuck either by calling stream.resume() to discard it. But you cannot destroy the stream as busboy will throw an error trying to continue process the next chuck in the same stream.

So do NOT try to do something like this, it will not work:

const resolve = await Promise.all(data.pictures);
resolve.map(...)

@FreddyPoly is correct but the processFile is not consuming any data chuck and that's why parser stuck! Just call stream.resume on data event and it will work!

async processFile(file) {
    const resolved = await file;

    if (!(resolved.stream instanceof stream.Readable)) return new Error('Wrong file');
    if (!(/\.(gif|jpg|jpeg|tiff|png)$/i).test(resolved.filename)) return new Error('The picture must be a valid file (gif, jpg, jpeg, tiff, png)');

    return null;
  },
async filesValidator(data) {
    if (!data.pictures) return new Error('An offer needs at least 1 picture to be created');
    if (data.pictures.length > 5) return new Error('A maximum of 5 pictures per offer is allowed');

    await Promise.all(data.pictures.map((pic) => {
      this.processFile(pic.file);

      return pic;
    }));
    // gets executed before the end of the map loop

    return null;
  },

By the way, you may do what you want in a for-loop but not recommended as small files will finish their job faster, write it in async way will be much more efficient. If you hack the middleware.js to log the "end" event, you will find that small files finish much faster and using async should save more resources.

[FreddyPoly]

Yes indeed if I want the loop to process all the files I need to consume the stream for each one. Changing my code to:

for (const pic of data.pictures) {
  const resolved = await pic.file;

  if (!(resolved.stream instanceof stream.Readable)) return new Error('Wrong file');
  if (!(/\.(gif|jpg|jpeg|tiff|png)$/i).test(resolved.filename)) return new Error('The picture must be a valid file (gif, jpg, jpeg, tiff, png)');

  resolved.stream.resume();
 }

Makes the for...of loop process al the files in an async way. Just as I wanted it to behave.

However, later I need to get the stream once again to upload the picture to AWS. But this time the stream is empty. I guess that if I consume the stream once it gets empty the next time ?

Can someone confirm this assumption of the empty stream ? I guess I'll have to change my workflow if that's the case.