Stop wire close from SMBRawIO finalizer#353
Conversation
The inherited io.IOBase.__del__ calls close(), which sends an SMB2 CLOSE and waits on Connection.receive(). When GC schedules the finalizer on the SMB worker thread, the wait self-deadlocks: only the worker can set the event it is waiting on.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
=======================================
Coverage 98.78% 98.78%
=======================================
Files 24 24
Lines 5273 5285 +12
=======================================
+ Hits 5209 5221 +12
Misses 64 64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return | ||
| if closed: | ||
| return | ||
| _warn(f"unclosed SMB handle {self._name!r}", ResourceWarning, source=self) |
There was a problem hiding this comment.
_warn doesn't seem to be defined so it sounds like this would fail? Is the goal for this to be a no-op with the warning added?
There was a problem hiding this comment.
_warn is defined at line 428.
Yes, it's supposed to be warning and no-op.
This means a resource leak, not good but better than deadlock.
There are multiple locations that needs fixes in the error paths to avoid that and I plan follow-up PR's once this is merged.
There was a problem hiding this comment.
Is there a reason why you used the pattern to define the function as a kwarg default? Why not just do warnings.warn(...) instead?
There was a problem hiding this comment.
The pattern was copied from subprocess.Popen.__del__. I guess there is a theoretical chance the "warnings" is teared down at the time of calling del if not holding a reference. But I can remove that part if you want me to? I have not seen it happen.
|
Anything I can do to help to git this merged? The issue has a reproducer, it will trigger a bunch of other issues that I have fixes for once this is merged. |
|
Sorry I've been away for a bit so have had limited GitHub interaction. Just added another reply to the change. |
2 participants
Fixes #352.
io.IOBase.del would call self.close() during GC, which
issues SMB I/O and waits on Connection.receive(). On the
worker thread that wait self-deadlocks. Override
SMBRawIO.del to skip the wire close and emit
ResourceWarning instead, mirroring asyncio.BaseEventLoop
and subprocess.Popen, which warn rather than perform
cleanup that would be unsafe in the finalizer's thread.
Add a Connection.receive guard that raises SMBException if
called from the worker thread, so the same mistake fails
loudly elsewhere.
The warning fires only when an SMBRawIO is dropped without
being closed. The usual idioms (with open_file(...) as f:
or explicit f.close()) prevent it; the smbclient.shutil
helpers manage their own handles.