Repository navigation
Don't mask open errors in the filesystem transport - #2683
Conversation
_put() and _queue_bind() opened their file inside a try whose finally unconditionally unlocked and closed it. When open() failed, for example because data_folder_out does not exist, the finally raised UnboundLocalError and hid the real error, including the ChannelError _put() means to raise. Only unlock and close the file once it was opened, as _delete() already does.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2683 +/- ##
==========================================
+ Coverage 85.02% 85.06% +0.04%
==========================================
Files 80 80
Lines 11317 11322 +5
Branches 1354 1356 +2
==========================================
+ Hits 9622 9631 +9
+ Misses 1475 1471 -4
Partials 220 220 ☔ View full report in Codecov by Harness. |
MehrazRumman
left a comment
MehrazRumman
left a comment
There was a problem hiding this comment.
Thanks @kratos0718, small and correct.
- In
_put()and_queue_bind(), a failingopen()leftf/f_objunbound, so thefinallyraisedUnboundLocalErrorand hid the real error. For_put()that also meant the intendedChannelErrornever reached the caller. Initialising toNoneand only unlocking/closing when the handle exists matches what_delete()already does. - The tests reproduce both paths realistically (removing
data_folder_out; putting a directory where the exchange file should be) rather than mockingopen, and both would fail onmainwithUnboundLocalError.
Minor, non-blocking:
- Inconsistent error types. After this change, an open failure in
_put()becomesChannelError, while the same failure in_queue_bind()propagates as a rawOSError(andget_table()convertsOSErrortoChannelError). SinceChannelErroris in the transport'schannel_errors, which callers such as Celery know how to handle, wrapping_queue_bind()'s open failure the same way would make the transport consistent. Either way, the test nametest_queue_bind_propagates_open_errordocuments the current behaviour. - Chaining.
raise ChannelError(f'Cannot add file {filename!r} to directory')insideexcept OSError:has nofrom exc, so the message doesn't say why (permission denied, missing directory, ...). The original error is still attached as__context__, butraise ... from exc(or includingexc.strerrorin the message) would make logs clearer. - A similar leak nearby (pre-existing). In
_get(),f = open(...); payload = f.read(); f.close()leaks the handle ifread()fails. Awith open(...) as f:would fix it if you want to tidy that up in the same spirit, but it's fine as a follow-up.
LGTM.
these should be fixed in this pr for hardening this @kratos0718 |
…e in _get - _queue_bind now wraps a failure to open the exchange file in ChannelError like _put and get_table, so callers get the transport's channel error in every case. - every ChannelError raised for an OSError now chains it with 'from exc', so the log says why the file could not be opened. - _get reads the message file in a with block, so the handle is closed when read() fails.
|
Done in the latest commit. |
Channel._put()andChannel._queue_bind()in the filesystem transport open their file inside atrywhosefinallyunconditionally unlocks and closes it. Ifopen()fails, thefinallyraisesUnboundLocalErrorand hides the real error. For_put()that also swallows theChannelError('Cannot add file ...')it is meant to raise. Adata_folder_outthat does not exist is enough to hit it:This initialises the handle to
Noneand only unlocks and closes it once it was opened, the same way_delete()already does. I added two tests int/unit/transport/test_filesystem.pythat fail on main with theUnboundLocalErrorand pass with the change. The unit suite and pre-commit pass.