New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clearer service exceptions #1245
Conversation
- if child service can't start - ValidationError mildly more helpful than RuntimeError Also, prefer run_child_service over run_task, because it has more info to give better error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marked as "request changes" due to concern over unbounded growth of the child services set. If this is not an issue 👍 on this PR.
@@ -732,7 +732,7 @@ def unsubscribe(self, subscriber: PeerSubscriber) -> None: | |||
peer.remove_subscriber(subscriber) | |||
|
|||
async def start_peer(self, peer: BasePeer) -> None: | |||
self.run_task(peer.run()) | |||
self.run_child_service(peer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems problematic as I don't see a mechanism for peers which stop to be removed from the self._child_services
set, which will result in that set growing in an unbounded nature. I know that the task
set uses a WeakSet
to ensure that old tasks are garbage collected (which I'm realizing we should maybe validate that at some point?).
987dcb0
to
0961371
Compare
Weird, my |
0961371
to
232980a
Compare
You probably missed #1229 |
Ah yes, thanks @cburgdorf . I'm actually using |
Wait, it's worse/weirder than that, the tox envlist didn't get changed either. |
Oops 🙈🙉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a test in the tests/p2p
module somewhere that validates that completed tasks and child services do indeed get cleared from the list. I'm now paranoid we think these will be automatically cleaned up but that they won't.
👍 done |
What was wrong?
When child services couldn't start (in
run_child_service
orrun_daemon
), the stack had lost who the calling parent was because of asyncio indirection.How was it fixed?
Also, prefer
run_child_service
overrun_task
in peer, because it has more infoto give better error messages.
Cute Animal Picture