useTempFiles option does not clean up after request

[AlexSpelt]

useTempFiles option does not clean up after request

The package in the current state does not have an option to clean up temporary files after the response(With an exception to an uploadTimeout). In my opinion, cleaning up files should be the default behaviour. My application keeps on piling up files in the temporary files folder. If the application is run in a production environment then the maximum storage capacity would be compromised.

my fix

I solved this issue like this: server.ts

app.use(fileUpload({
    useTempFiles: true,
    tempFileDir: './temp/'
}));

// remove all temporary uploaded files from temp folder after response has been sent.
app.use((req, res, next) => {
    res.on('finish', () => FileUploadHelper.clearTempFiles(req));
    next();
});

FileUploadHelper.ts

/**
 * This method will delete all temporary uploaded files from the request
 * 
 * @param req express request obj
 */
public static clearTempFiles(req: Request) {
    if(req.files) {
        if(Array.isArray(req.files.files)) {
            req.files.files.forEach((file) => fs.unlink(file.tempFilePath));
        } else {
            fs.unlink(req.files.files.tempFilePath);
        }
    }
}

You can also delete all files from the temp folder at a time where the application has the least used. But that can result in other errors.

[RomanBurunkov]

That sounds crazy for me.

Why middleware should cleanup folder after responding to the request? IMHO it is up to your's app logic.

There are some cases to cleanup temporary files, e.g. one you mentioned when upload stuck and timeout triggered. But if you don't use mv method there are so many things some others probably wants to do with uploaded files so deleting all of them after responding to the request by default will be 100% confusing.

[AlexSpelt]

It sounds quite logical to me that a temporary file should be temporary. Adding an option for a default upload folder next to a temp file option would be a more logical option. But that would be a breaking change for many apps using this package.

Adding an option to be able to remove an tempfile after the request has been responded to would be a nice addition.

[r3wt]

@AlexSpelt it is not possible to remove temp files in node.js. even if you unlink the file, it doesn't get deleted until the process exits. its a known issue with node.js. the most common workaround is to use fs.truncate() in conjunction with fs.unlink() to reclaim space in temp directory, with knowledge file won't be deleted until process exit. here's an example you can use to get you started

const tmp_dir = '/tmp';// path to your temp file directory
fs.readdir(tmp_dir,{withFileTypes:true},(err,files)=>{
    if(err){
        console.warn('unable to read temp files directory');
        return;
    }
    if(Array.isArray(files)){
        var time = (new Date()).getTime();//get ms since epoch
        //because of withFileTypes option, files are fs.Dirent objects instead of just string filenames.
        files.forEach(file=>{
            //make sure its a file before proceeding
            if(file.isFile()){
                fs.stat(tmp_dir+file.name,(err,stats)=>{
                    if(err){
                        console.warn('unable to fs.stat() file %s',file.name);
                        return;
                    }
                    //if the time the file created is greater than or equal to 1 hour, delete it
                    if(stats.birthtimeMs - time >= 3.6e+6) {
                        fs.truncate(tmp_dir+file.name,()=>{
                            fs.unlink(tmp_dir+file.name,err=>{
                                if(err){
                                    console.warn('unable to remove temp file %s',file.name)
                                }else{
                                    console.log('temp file %s removed',file.name);
                                }
                            });
                        })
                    }
                })
            }
        })
    }
});

[RomanBurunkov]

Hi @r3wt ,

I haven't faced with that node issue, could u please point me on it.

[RomanBurunkov]

@AlexSpelt , removing temporary file already included into 'mv' method of the file object. If u don't use 'mv', you always can do it manually and I assume that is what you have in your app.

Probably in your case that is really convenient and useful, but let me put it in this way.

Logically middleware shouldn't do anything after it is done with a next. It helps to upload files and gives you files objects for further processing and that is it.

So adding some extra logic for res finish event will obviously broke that logic and could lead to unexpected behavior.

[AlexSpelt]

I think I found the issue that @r3wt mentioned. https://stackoverflow.com/questions/48529030/nodejs-fs-unlink-not-releasing-file-handles.

I don't store my files on the same server. My application runs on a Linux server. However, some of my files need to be processed by a windows only application. I upload all my file uploads to the windows server.

@RomanBurunkov I will close this issue because you are right about middleware's purpose. I was not really aware about that. I, however still stand by my statement about the name of the useTempFile option. In my opinion, the option should either be renamed to for example uploadFolder or useTempFile should be documented better.

Also, the 'mv' method you mentioned. The delete part is only documented in the source code. It is overwritten sadly by the @types/express-fileupload for me.