Fix a never-ending subtask in BasePeerPool #1310
Conversation
The handle_peer_count_requests() coroutine runs as a background task but did not exit when the pool's cancel token was triggered. Also introduce a custom log formatter for trinity that uses the last element of the logger's name instead of the module, otherwise it's not clear where they're coming from (specially for those generated by BaseService subclasses).
| async def f() -> None: | ||
| # FIXME: There must be a way to cancel event_bus.stream() when our token is triggered, | ||
| # but for the time being we just wrap everything in self.wait(). | ||
| async for req in self.event_bus.stream(PeerCountRequest): |
There was a problem hiding this comment.
@cburgdorf is there a way to do this already?
There was a problem hiding this comment.
Oh..so one thing you can do with the stream API is to break out e.g.
async for req in self.event_bus.stream(PeerCountRequest):
if not self.is_operational:
breakThis will then be taken care of to handle the unsubscribing.
except GeneratorExit:
self._queues[event_type].remove(queue)However, as I'm writing this, I realize this is probably not what you want because it only allows you to break out at the moment where another event comes through.
I think what we really want is to be able to pass a CancelToken as in.
async for req in self.event_bus.stream(PeerCountRequest, cancel_token=token):
...Does that make sense?
There was a problem hiding this comment.
Yep, passing a CancelToken is what I had in mind. Also, as you pointed out, doing the is_operational check and breaking out of the loop is not going to fix it
There was a problem hiding this comment.
Alright, there are a bunch of improvements that I'd like to get in for a next release soon. If we can live with the stopgap approach of this PR for a few days I'll come up with a new release soon that will have this API change.
I created an issue for this particular problem. ethereum/lahja#9
We now use the TRACE level for most discovery/kademlia logs, so we no longer need to force their level to INFO in trinity Closes: ethereum#1243
gsalgado
force-pushed
the
fix-peerpool-neverending-subtask
branch
from
e9265c7
to
29b7e3b
Compare
The handle_peer_count_requests() coroutine runs as a background task but
did not exit when the pool's cancel token was triggered.
Also introduce a custom log formatter for trinity that uses the last
element of the logger's name instead of the module, otherwise it's not
clear where they're coming from (specially for those generated by
BaseService subclasses).