twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 406 forks source link

EnrichedResponse.file leaks open files #440

Closed trym-moeller closed 6 years ago

trym-moeller commented 6 years ago

EnrichedResponse.file opens a file, but nothing seems to close it again

Expected behavior

When returning a file in a Finatra Controller, I expect the returned file to be closed when the request/response is finished.

I am using code as follows

import com.twitter.finagle.http.Request
import com.twitter.finatra.http.Controller

class TestController extends Controller {  
  get("/myfile.json") { _: Request => response.ok().file("myfile.json") }
}

Which is using

https://github.com/twitter/finatra/blob/develop/http/src/main/scala/com/twitter/finatra/http/response/ResponseBuilder.scala

Actual behavior

The current code leaves the file open in jvm increasing the number of open files on the OS.

Steps to reproduce the behavior

Add a controller to a finatra server as described above, invoke the controller (e.g. in a browser /myfile.json), try to delete the file (on Windows this gives an error message like "cannot delete an open file") or on a more fun OS count the number of open files before and after.

cacoco commented 6 years ago

@trym-moeller are you running in local file mode? E.g., with the -local.doc.root set?

trym-moeller commented 6 years ago

@cacoco I have not explicit set the -local.doc.root, so it must have its default value. Best regards Trym

cacoco commented 6 years ago

@trym-moeller Ok. Addressed in dafe7259b2ec3d89641425e3eca8af56a869b4e3. Let us know if you still encounter issues. Thanks!

trym-moeller commented 6 years ago

Thanks @cacoco How can I try the fix (is there a build artifact containing the fix)?

cacoco commented 6 years ago

@trym-moeller it'll be in the next release (in January). Otherwise you can build the current develop branch locally following the guidelines in the CONTRIBUTING.md by first building the dependencies, then building Finatra.