Prevent libev watchers running on closed socket#903
Conversation
`close` can be called from anywhere, not only reactor threads. If such `close` call closes socket during `handle_write` / `handle_read`, then those functions may try to operate on closed socket. Solution implemented in this commit: defer socket closing until both watchers are stopped.
Previous commit defered socket close until watchers are stopped, but there is one more case worth considering. If during one libev loop iteration socket gets ready for both read and write, then both watchers will be called. If one decides to close the connection, the other one will still get called anyway. This shouldn't cause EBADF, because socket won't be closed yet, but I see no reason to perform unnecessary work.
When connection is closed by the server, but there is no other error, it will be close (is_cloes == True) without setting `last_error`. This is true for all reactors apart from Twisted as far as I can tell. If we try to use such connection, we'll quickly discover that its broken, but we can slightly optimize this process by raising directly from factory().
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR addresses socket lifecycle management and server-initiated connection closure handling in the Python driver. The connection factory now detects when the server closes a connection after the connection event completes and raises 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The way I understand Connection semantics,
closecan be called from any thread at any time.It may happen when
handle_write/handle_readare already running. Ifclosecloses the socket, and one of watchers uses it, we may getEBADF.Apart from this specific error, it just seems conceptually weird that resources operating on socket (watchers) are closed later than the socket itself.
The solution for this issue that I implemented here is quite simple: close the TCP socket only after watchers are stopped.
closecallsconnection_destroyed, which registers connection to be destroyed. Some time later,_loop_will_runis called which goes trough all connections that are registered to be destroyed, and stops their watchers. I moved socket close fromcloseto after watchers are stopped for the connection in_loop_will_run.Additionally, I took the early returns from PRs of @vponomaryov and @fruch .
I'm not 100% convinced they are necessary for correctness, but:
I decided to not take @vponomaryov change that sets
last_errorwhen connection is gracefully closed by the server. After a brief look at other reactors, it looks like its an established convention that graceful close does not setlast_error- only Twisted does not abide by this.I did pick up another optimization, in slightly changed form:
factorynow checks if connection is closed withoutlast_error, and raises if so.This is not necessary for correctness at all imo, because the connection may get closed just after being returned from factory, so we are not preventing any scenarios. It may however be an optimization in some cases, because we'll learn quicker that connection is dead.
Fixes: #614 (hopefully)
Pre-review checklist
I added relevant tests for new features and bug fixes.I have provided docstrings for the public items that I want to introduce.I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.