Upload of two and more scalars

[dgorobets]

Hi! Thanks for repository.

I'm having a problem with uploading of two scalars.

My input with two scalars for mutation is:

input createTrainingInput {
  name: String!
  level: TrainingLevel!
  introduction: String!
  workoutDuration: String!
  description: String!
  goals: [String]!
  requirements: [String]!
  overallDuration: Int!
  intensity: Int!
  price: Int!
  promotionalVideo: Upload!
  coverPhoto: Upload!
}

When I upload small plain text files, everything is ok.

But when I'm trying to upload video one of the promises is in 'pending' state for a long time, until FileStreamDisconnectUploadError is thrown.

Resolvers code:

const { coverPhoto, promotionalVideo, ...properties } = input;

const [
    { stream: coverPhotoStream, filename: coverPhotoFilename },
    { stream: promotionalVideoStream, filename: promotionalVideoFilename },
] = await Promise.all([
    coverPhoto,
    promotionalVideo
]);

Version of apollo-upload-server is 5.0.0.

Please, advice what can be done in this situation.

Thank you.

[jaydenseric]

Files stream into the server one after the other, so if the first file is big the promise for the following one will be pending until it is done. This is how it's meant to work. Why in your case it is disconnecting I can't say, there could be lots of server-specific reasons. Perhaps there are some connection timeout settings, etc.

[dgorobets]

I tried to upload multiple files in one parameter as in example, and it worked. But if I send files using different parameters, it's stuck until exception.

[hkjeffchan]

Same here. Error occurs when it is sent via one two parameter in GQL.

type Mutation {
  upload2File(file1: Upload, file2: Upload): Boolean
}

If two small files are sent, there is no problem. But if the file size is large (~4MB for 2 files in my case), the following error occurs:

/Applications/MAMP/htdocs/nestjs/node_modules/apollo-upload-server/lib/middleware.js:151
new _errors.FileStreamDisconnectUploadError(
^
FileStreamDisconnectUploadError: Request disconnected during file upload stream parsing.
at IncomingMessage.request.on (/Applications/MAMP/htdocs/nestjs/node_modules/apollo-upload-server/lib/middleware.js:151:15)
at emitNone (events.js:106:13)
at IncomingMessage.emit (events.js:208:7)
at abortIncoming (_http_server.js:413:9)
at socketOnClose (_http_server.js:406:3)
at emitOne (events.js:121:20)
at Socket.emit (events.js:211:7)
at TCP._handle.close [as _onclose] (net.js:554:12)

[hkjeffchan]

Some more information. Test sending two files (A = 25kb, B = 5.4MB).

If file list = [A, B], it can send correctly.
If file list = [B, A], it can cause the FileStreamDisconnectUploadError.

Any idea?

[hkjeffchan]

By the way, it was in a localhost testing environment so it is probably not a timeout issue here.

[jaydenseric]

@hkjeffchan I don't have any ideas sorry.

The steps to getting this fixed is to first add a new test demonstrating a failure, then we can work on a solution to get the test to pass. Would you be keen to submit a PR? Here is a test for reference.

[hkjeffchan]

@jaydenseric
I did try to write the test but really don't know how I can demonstrate that in a test. It seems that it is related to promise.all resolve issue.

The following works but it does not really catch any error.

        fileList.map(file => {
            file.then(res => storeFS(res.stream, res.filename));
        });
        return true;

The following only work on small files.

        const promise = new Promise((resolve, reject) => {
            Promise.all(fileList).then(
                files => {
                    files.map(res => storeFS(res.stream, res.filename));
                    resolve(true);
                },
                error => reject('error uploading'),
            );
        });
        return promise;

[hkjeffchan]

After hours of research, I finally found the solution.

We cannot do something like this:

await Promise.all([
    coverPhoto,
    promotionalVideo
]

Instead, we should create a new Promise for each desired operation and await Promise.all for all operations.

[jaydenseric]

@hkjeffchan makes perfect sense. I think the example resolver for multiple files is still correct though?

[hkjeffchan]

@jaydenseric
Yes, the example is correct. It passes the promises of the operation processUpload to promiseAll instead of awaiting to resolve all the stream and apply the operations later. The stream won't resolve as it does not have anything to do.

I am not sure if this is 100% correct but I think it is similar to rxjs. The stream is more or less like a cold subject and if no observer subscribed it, it will not do anything and therefore will never resolved.

[dgorobets]

@hkjeffchan

Can you point me to the right direction? I'm still can't understand how to handle two or more files.

My resolver code is:

createTraining: async (root, { input }, context) => {
    const { user } = context;
    const coach = await Coach.findOne({ user });
    const { coverPhoto, promotionalVideo, ...properties } = input;

    const { stream: coverPhotoStream, filename: coverPhotoFilename } = await coverPhoto;
    const { stream: promotionalVideoStream, filename: promotionalVideoFilename } = await promotionalVideo;

    const photoFilename = getUploadPath('training_videos', coverPhotoFilename);
    const videoFilename = getUploadPath('training_cover', promotionalVideoFilename);

    await storage.save({
            stream: coverPhotoStream,
            filename: photoFilename
    });
    await storage.save({
            stream: promotionalVideoStream,
            filename: videoFilename
        });

    const training = new Training({
        coach,
        coverPhoto: coverPhotoFilename,
        promotionalVideo: videoFilename,
        ...properties
    });
    await training.save();
    return { training };
}

[hkjeffchan]

Do NOT do this, this will block the stream and will raise error.

    const { stream: coverPhotoStream, filename: coverPhotoFilename } = await coverPhoto;
    const { stream: promotionalVideoStream, filename: promotionalVideoFilename } = await promotionalVideo;

Wrap the stream operation in promise one by one, await all and then execute the rest operation. It should be something like this. I haven't tested it but hopefully you get the idea.

createTraining: async (root, { input }, context) => {
    const { user } = context;
    const coach = await Coach.findOne({ user });
    const { coverPhoto, promotionalVideo, ...properties } = input;

    await Promise.all(
         new Promise( (resolve, reject) => {
              const { stream: coverPhotoStream, filename: coverPhotoFilename } = await coverPhoto;
              const photoFilename = getUploadPath('training_videos', coverPhotoFilename);
              await storage.save({
                      stream: coverPhotoStream,
                      filename: photoFilename
              });
              resolve(true);
         }),
         new Promise( (resolve, reject) => {
             const { stream: promotionalVideoStream, filename: promotionalVideoFilename } = await promotionalVideo;
             const videoFilename = getUploadPath('training_cover', promotionalVideoFilename);
             await storage.save({
                   stream: promotionalVideoStream,
                   filename: videoFilename
             });
             resolve(true);
         })
    );

    const training = new Training({
        coach,
        coverPhoto: coverPhotoFilename,
        promotionalVideo: videoFilename,
        ...properties
    });
    await training.save();
    return { training };
}

[koresar]

The code above can be simplified.

• Do not create promises explicitly - new Promise()
• Instead, it will be created for you. - The await keyword will create that promise for you under the hood.

createTraining: async (root, { input }, context) => {
    const { user } = context;
    const coach = await Coach.findOne({ user });
    const { coverPhoto, promotionalVideo, ...properties } = input;

    async function uploadFile(file, path) {
              const { stream, filename } = await file;
              const photoFilename = getUploadPath(path, filename);
              await storage.save({
                      stream: coverPhotoStream,
                      filename: photoFilename
              });
    }

    await Promise.all([
      uploadFile(coverPhoto, 'training_cover'),
      uploadFile(promotionalVideo, 'training_videos')
    ]);

    const training = new Training({
        coach,
        coverPhoto: coverPhotoFilename,
        promotionalVideo: videoFilename,
        ...properties
    });
    await training.save();
    return { training };
}