Lifecycle fixes#151
Open
gonzalocasas wants to merge 3 commits into
Open
Conversation
TwistedEventLoopManager.__init__ instantiated PythonLoggingObserver and started it; the observer was only stopped in manager.terminate(), which in turn was only invoked by Ros.terminate() (which stops the reactor permanently). Downstream code that follows the documented RosClient.__exit__ -> Ros.close() pattern never called terminate(), so every Ros instance added a permanent observer to twisted's globalLogPublisher. In long-running pytest sessions that create many short-lived Ros instances (e.g. compas_fab's integration suite), this accumulated tens of observers and slowed every twisted log emission linearly. It also contributed to occasional RosTimeoutError flakes on the next Ros.run() after many cycles. Make the observer a process-wide singleton: TwistedEventLoopManager lazily starts a single module-level PythonLoggingObserver via a thread- safe initializer, and terminate() now stops it once and clears the sentinel. Verified by a new tests/ros1/test_lifecycle.py case that constructs 10 Ros instances and asserts at most one observer is added. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously Ros.close() returned as soon as proto.send_close() had been dispatched onto the reactor thread: it set wait_disconnect inside the on_ready callback, before clientConnectionLost ever fired. Downstream callers (compas_fab's RosClient.__exit__ -> Ros.close() -> immediately construct a new Ros) could then race the previous socket's TCP teardown, and on contended reactors the next Ros.run() occasionally tripped its 10s connection timeout. close() now registers a one-shot listener on the factory's "close" event (emitted from clientConnectionLost when the underlying TCP connection has actually been lost) and only sets wait_disconnect from there. The "closing" event still fires synchronously inside the on_ready callback before send_close(), so handlers that depend on that ordering are unaffected. If the disconnect doesn't complete within the timeout the listener is detached before raising RosTimeoutError, so a subsequent reconnect cycle on the same factory won't leak a reference to this Ros instance. Verified by a new test_close_blocks_until_disconnect case in tests/ros1/test_lifecycle.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've seen
roslibpyfailing to connect when used to connect and disconnect multiple times (usually on CI during integration tests), so this PR tries to address that problem.What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.rstfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke check).