keichan34 / exfile

File upload persistence and processing for Phoenix / Plug
MIT License
90 stars 19 forks source link

Sendfile doesn't work with Exfile.Tempfile in Cowboy 2 #64

Closed scarfacedeb closed 5 years ago

scarfacedeb commented 5 years ago

After I updated cowboy to v2, I discovered that most of the processed files served by Exfile stopped rendering.

It only happens with requests that have image processing specified in the url.

I think that the issue is caused by the combination of Exfile.Tempfile use for the image processing and Plug.Conn.send_file/3 for sending the response.

I created an example app to narrow down the issue: https://github.com/scarfacedeb/exfile_sendfile_example. master branch uses cowboy 2 and has the issue. cowboy1 branch uses cowboy 1 and doesn't have it.

Explanation

  1. Exfile.Router accepts the request for attachment with processing, (e.g. http://localhost:4422/attachments/1b4956bf4d77aa6ddd3c3d68272dd6683cb7cc37/store/limit/160/320/b81d4a3cc45d2ae38b601cdd6587ac40087ab89f4b2750fbefb1d5832f0e/thumb.png)
  2. Limit processor creates new temporary random file for its results limit.ex:27
  3. Plug.Conn.send_file/3 initiates sending the temp file router.ex:169 and cowboy delegates the sendfile call to the kernel which begins to actually send the data to the client.
  4. Meanwhile, Cowboy destroys the request process and Exfile.Tempfile deletes the temp file tempfile:103
  5. Client can't receive anymore data, because the temp file is no longer available.

Steps to reproduce

  1. Upload an image to the store
  2. Make a request to the attachment url with any processing (e.g. limit)
  3. Check the response, it'll likely say transfer closed with n bytes remaining to read (in case of curl).

Possible solutions

  1. Switch to Plug.Conn.send_resp/3 calls to send the binary data from elixir itself.
  2. Add separate cleaning process that will delete the temp files later

I don't particularly like any of them, but I'll think about better workarounds later.

keichan34 commented 5 years ago

I see... So Cowboy 2 is killing the response process before the whole file has been completely sent, while Cowboy 1 kept the process around until the file was finished?

Looking at the Cowboy code, it looks like there's a way to disable the kernel sendfile and fall back to a compatibility layer, I wonder if disabling it would result in the same problem?

https://github.com/ninenines/cowboy/blob/7ff9e963b89c50f29a33e3e72bbea16d0f26affa/src/cowboy_http2.erl#L681

Anyways, I suppose the best way right now is the way you've implemented it in the commit -- I really don't want to keep track of old files and clean them up occasionally -- that just seems like a lot more complexity. It might come down to that, though...