-
Notifications
You must be signed in to change notification settings - Fork 248
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
chainhead backend: notify subscribers when the backend is closed #1817
Conversation
|
Looking at the PR, I wonder about a couple of things:
I'd be up for seeing whether (1) was possible anyways or whether it adds a bunch of complexity or something that I'm not thinking about! |
I suppose we could Arc it or something to work around that though... I guess it's not super useful with that error on driver except whether it was disconnecting_will_reconnect |
Yep; I like your idea of the |
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.
Nice!
Close #1808
This PR fixes that once Shared::done then we also wake up the "waker" such that the FollowStreamSubscription poll impl gets notified about.
This issue was really that the FollowStreamSubscription wasn't woken up after that
Shared.done == true
was set.Currently the backend only returns error once before it's shutdown and then return None but this forces users of the driver to run it completion and one doesn't run it to completion i.e, such as bailing on error then Shared::done == true is never set.
Thus, I also had to change that we run the driver until completion.
Since we already now that backend is shutdown when it returns error we could already set
done == true
here but it's doesn't feel correct to me but would probably help users of the low-level API not run into such tricky issues ^^, thoughts?