Skip to content
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

Make sure LocalFile handles get closed #489

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

garrettjstevens
Copy link
Contributor

As described in #488, I was getting this error when loading data on the demo server:

    Error: importFeatures failed — 422 Unprocessable Entity 
    ({"message":"Error: EMFILE: too many open files, open 
    '/data/uploads/05dc2a73502099956dce87e417ed886c'","error":"Unprocessable 
    Entity","statusCode":422})

It turns out #488 did not fix this, so after some more digging I found the solution in this PR. It turns out we have to be a bit careful about making sure we close filehandles when using LocalFile so that we don't have a lot of opened files.

@cmdcolin
Copy link

cmdcolin commented Dec 4, 2024

not sure if it's relevant but I was considering modifying the way LocalFile works in GMOD/generic-filehandle#114 to not maintain an open filehandle. this way it would close and re-open for each read. while it could result in slightly less performance (benchmark could be made) it would reduce the need to meticulously manage the open/closing of the file (it is implicitly opened....explicitly closed as in this PR)

@cmdcolin
Copy link

cmdcolin commented Dec 4, 2024

(that particular change could be made without the other Uint8Buffer related API adjustment that is proposed)

@garrettjstevens
Copy link
Contributor Author

garrettjstevens commented Dec 4, 2024

That could also work. One thing I considered was using an LRU cache of filhandles instead of closing each one, but I didn't try it since, same as you mentioned, I wasn't sure what the performance benefit would be for that added complexity.

@garrettjstevens garrettjstevens merged commit 0f2819c into main Dec 4, 2024
7 checks passed
@garrettjstevens garrettjstevens deleted the close_file_handles branch December 4, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants