fix(files): close mmap in abstract file _prefetch #70707
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #70707 +/- ##
===========================================
+ Coverage 50.46% 80.37% +29.91%
===========================================
Files 6469 6517 +48
Lines 289328 298593 +9265
Branches 49888 52539 +2651
===========================================
+ Hits 146002 239994 +93992
+ Misses 142890 57865 -85025
- Partials 436 734 +298
|
|
looking at this some more, the sockets are network based, not file based, so i don't think this PR is correct. looking further |
|
gonna guess its coming from https://github.com/getsentry/getsentry/blob/056e9cc337b98984b37ffd3b40a79adea07c32e5/getsentry/filestore.py#L141 |
| @@ -107,6 +107,7 @@ def fetch_file(offset, getfile): | |||
| exe.submit(fetch_file, idx.offset, idx.blob.getfile) | |||
|
|
|||
| mem.flush() | |||
| mem.close() | |||
There was a problem hiding this comment.
mmap supports the context manager protocol so I would recommend that instead
mmap also doesn't trigger a ResourceWarning when left unclosed (though arguably it should) so your warnings are definitely coming from somewhere else -- do you have some sample lines?
There was a problem hiding this comment.
yeah, i think its coming from the getfile function that is passed in (https://github.com/getsentry/getsentry/blob/056e9cc337b98984b37ffd3b40a79adea07c32e5/getsentry/filestore.py#L141 in prod)
and example warning is:
/usr/src/sentry/src/sentry/models/files/abstractfile.py:38: ResourceWarning: unclosed <socket.socket fd=179, family=2, type=1, proto=6, laddr=('10.4.161.44', 43014), raddr=('10.8.0.230', 11211)>
self._prefetch(prefetch_to, delete)
|
this should probably happen anyway -- right now that file descriptor is torn down at an undefined time |
we seem to have a lot of warnings about this function and unclosed sockets. reading python docs, it seems like we should close this mmap. I'm not quite sure if this is the source of the log.
https://cloudlogging.app.goo.gl/Yu321395kTNxZFiWA