fix(temporaryfilestorage): streams of temporary files are now properl…

…y closed (#309)
Lumieducation · Dec 5, 2019 · 72ef6cb · 72ef6cb
1 parent f7f4244
commit 72ef6cb
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 18 deletions.
diff --git a/examples/express.ts b/examples/express.ts
@@ -72,6 +72,7 @@ const start = async () => {
         );
         stream.on('end', () => {
             res.end();
+            stream.close();
         });
         stream.pipe(res.type(path.basename(req.params.file)));
     });
@@ -86,6 +87,7 @@ const start = async () => {
             );
             stream.on('end', () => {
                 res.end();
+                stream.close();
             });
             stream.pipe(res.type(path.basename(req.params.file)));
         }

diff --git a/src/ContentStorer.ts b/src/ContentStorer.ts
@@ -262,22 +262,26 @@ export default class ContentStorer {
                     `Temporary file ${fileToCopy} does not exist or is not accessible to user: ${error}`
                 );
             }
-            if (readStream !== undefined) {
-                log.debug(
-                    `Adding temporary file ${fileToCopy} to content id ${contentId}`
-                );
-                await this.contentManager.addContentFile(
-                    contentId,
-                    fileToCopy,
-                    readStream,
-                    user
-                );
-                if (deleteTemporaryFiles) {
-                    await this.temporaryFileManager.deleteFile(
+            try {
+                if (readStream !== undefined) {
+                    log.debug(
+                        `Adding temporary file ${fileToCopy} to content id ${contentId}`
+                    );
+                    await this.contentManager.addContentFile(
+                        contentId,
                         fileToCopy,
+                        readStream,
                         user
                     );
+                    if (deleteTemporaryFiles) {
+                        await this.temporaryFileManager.deleteFile(
+                            fileToCopy,
+                            user
+                        );
+                    }
                 }
+            } finally {
+                readStream.close();
             }
         }
     }

diff --git a/src/TemporaryFileManager.ts b/src/TemporaryFileManager.ts
@@ -69,6 +69,7 @@ export default class TemporaryFileManager {
     /**
      * Returns a file stream for temporary file.
      * Will throw H5PError if the file doesn't exist or the user has no access permissions!
+     * Make sure to close this stream. Otherwise the temporary files can't be deleted properly!
      * @param filename the file to get
      * @param user the user who requests the file
      * @returns a stream to read from

diff --git a/test/H5PEditor.saving.test.ts b/test/H5PEditor.saving.test.ts
@@ -93,6 +93,7 @@ describe('H5PEditor', () => {
                 mockWriteStream1.on('finish', onFinish1);
                 await promisepipe(returnedStream, mockWriteStream1);
                 expect(onFinish1).toHaveBeenCalled();
+                returnedStream.close();
             },
             { keep: false, unsafeCleanup: true }
         );
@@ -360,12 +361,12 @@ describe('H5PEditor', () => {
                     0,
                     savedFilePath.length - 4
                 );
-                await expect(
-                    h5pEditor.temporaryFileManager.getFileStream(
-                        cleanFilePath,
-                        user
-                    )
-                ).resolves.toBeDefined();
+                const fileStream = await h5pEditor.temporaryFileManager.getFileStream(
+                    cleanFilePath,
+                    user
+                );
+                await expect(fileStream).toBeDefined();
+                fileStream.close();
 
                 // put path of image into parameters (like the H5P editor client would)
                 mockupParametersWithImage.image.path = savedFilePath;