search

Swirrl / drafter

A clojure service and a client to it for exposing data management operations to PMD
Other
0 stars 0 forks source link

Temp RDF files should be deleted promptly #150

Open lkitching opened 7 years ago

lkitching commented 7 years ago

RDF data which is appended to/deleted from a draftset is first written to disk by drafter.middleware.temp-file-body. This middleware should also delete the file after the inner handler has returned.

RickMoynihan commented 7 years ago

See also here:

IIRC we can't use the ring multipart file stuff because we need an implementation that works with a raw request body.

We should look to see if anyone has implemented such a thing, if not this would be a nice (but small OSS) contribution.

See also here for how ring multipart handles clean up:

https://github.com/ring-clojure/ring/blob/master/ring-core/src/ring/middleware/multipart_params/temp_file.clj

It might be worth actually reusing stuff in this implementation, or submitting a PR for it.

Also spoke to weavejester (the guy behind ring) and he said this would be a good middleware to opensource.

lkitching commented 7 years ago

This is complicated by the fact that most (all?) of the routes which use this middleware return async jobs which read the body file after the initial request (and therefore the temp-file-body middleware has returned). This means the middleware cannot currently delete the file it creates. One option would be to transfer ownership of the temp file to the calling middleware and therefore make the async jobs responsible for deleting the files. This approach has the disadvantage of making it easy to leak files as the same delete logic would be required in each async job.

Another approach could be to change async job middlewares to return the job directly instead of a job response. Jobs could then implement the ring Renderable protocol to return the required job response. If jobs could then allow continuation actions to be registered to execute on cleanup, the middleware could add a callback to delete the temp files in there. This approach requires a change to async jobs and therefore changes to lib-swirrl-server. Java already offers this kind of functionality in the Future interface so another possibility would be to change async jobs to encapsulate a Java Future rather than a clojure promise and execute them using a java Executor rather than a custom thread pool.