elwerene / libreoffice-convert

MIT License
253 stars 96 forks source link

Enabled parallel invocations #26

Closed shaharsol closed 4 years ago

shaharsol commented 4 years ago

The problem with parallel invocations was that the temp package cleanup was global, which caused it to remove dirs and files necessary for other concurrent invocations. Switched to use another very popular package -- tmp -- which has a more controllable cleanup.

One note about the code, since I must have access to the tmp dir object for cleanup in the final callback, I created the tmp dir outside of the async.task pile.

elwerene commented 4 years ago

@shaharsol Why did you close this merge request?

shaharsol commented 4 years ago

@elwerene because apparently I was wrong, and my solution didn't solve anything. I think you simply can't run libreoffice concurrently in the same process.

elwerene commented 4 years ago

https://github.com/elwerene/libreoffice-convert/issues/21#issuecomment-638559550

bgorhoball commented 4 years ago

I tried to have some adjustments below:

and I was able to see multiple "soffice.bin" running in my server. Hope this could help.

The part of code adjusted -

    installDir: callback => temp.mkdir('soffice', callback),
    saveSource: ['tempDir', (results, callback) => fs.writeFile(path.join(results.tempDir, 'source'), document, callback)],
    convert: ['soffice', 'saveSource', (results, callback) => {
        let command = `${results.soffice} -env:UserInstallation=file://${results.installDir} --headless --convert-to ${format}`;
elwerene commented 4 years ago

@shaharsol do you want to implement it? Or @bgorhoball do you want to integrate it into this pull request?

elwerene commented 4 years ago

@bgorhoball shouldn't you have the problem which @shaharsol described, that the temp directories are cleaned up globally?

bgorhoball commented 4 years ago

@elwerene yes, the tmp files did clean up eventually.

The temp.cleanup() was called in the callback function, so it's supposed they will only be cleaned after the conversion completed?

elwerene commented 4 years ago

@bgorhoball to my opinion it is a critical suggestion by @shaharsol that the cleanup is handled globally and could lead to problems on parallel invocation. Can you please merge his merge request into you branch?

elwerene commented 4 years ago

@shaharsol If you like, please send me your Name/Mail for the contributors info in package.json

shaharsol commented 4 years ago

Thanks @elwerene , must admit I ended up using another project named Gotenberg for my docx->pdf conversions, yet I'm glad I could be helpful and I think I can give libreoffice-convert a 2nd chance now.