-
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
return none if subscription returns early #250
Conversation
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
@@ -95,14 +95,7 @@ impl<'a, T: Runtime> EventSubscription<'a, T> { | |||
if self.finished { | |||
return None | |||
} | |||
let change_set = match self.subscription.next().await { | |||
Some(c) => c, | |||
None => { |
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.
The proper action should be to indicate that the subscription is dropped and need to restarted because after it returns None
it shouldn't be polled anymore because the channel has been terminated.
If you poll if after None
it will panic
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.
Which is exactly why I would expect it to return None
, to indicate the stream has closed (much like with StreamExt
).
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.
Alright, that makes sense to me but perhaps we should document it because it's not obvious to me ^^
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
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.
LGTM
I think our CI is broken, all test runs are hanging now, possibly due to recent |
I get this locally:
|
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
I don't think the CI issue is related to the client changes as it was working before on this pipeline, also worked locally for me with these latest changes. Is it possible it has something to do with the environment? There's an annotation on the last successful run that |
@gregdhill please merge master, it should fix CI |
Signed-off-by: Gregory Hill gregorydhill@outlook.com