New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EnrichedResponse.file leaks open files #440

Closed
trym-moeller opened this Issue Nov 21, 2017 · 5 comments

Comments

2 participants
@trym-moeller

trym-moeller commented Nov 21, 2017

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 cacoco self-assigned this Dec 11, 2017

@cacoco cacoco added the in triage label Dec 11, 2017

@cacoco

This comment has been minimized.

Member

cacoco commented Dec 11, 2017

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

@trym-moeller

This comment has been minimized.

trym-moeller commented Dec 12, 2017

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

@cacoco

This comment has been minimized.

Member

cacoco commented Dec 14, 2017

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

@cacoco cacoco closed this Dec 14, 2017

@trym-moeller

This comment has been minimized.

trym-moeller commented Dec 14, 2017

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

@cacoco

This comment has been minimized.

Member

cacoco commented Dec 15, 2017

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment