-
Notifications
You must be signed in to change notification settings - Fork 49
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
ObjStore empty list fix #572
Conversation
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.
Looking good 💯 just a quick question below.
await _commandTask; | ||
|
||
_msgChannel.Writer.TryComplete(); | ||
|
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.
why did we change these? did you encounter an issue? (you're probably right to make these changes, I just can't remember my reasoning here so just curious)
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.
Yes, I encountered some problem.
Before exiting WatchAsync, if Enumerable is empty, call pushConsumer.Init();
The consumer is initialized and the same consumer is immediately deleted in the dispose method.
The creation operation _consumerCreateTask
has not yet had time to complete and it requires an active _commandChannel
, but it is already closed. Based on this, I decided to change the order of stopping channel reading.
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.
Otherwise, when Dispose we get NatsJSApiException: consumer not found
.
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.
awesome, thanks fixing that 💯 should we stick this as comment on top of the relevant lines do you think?
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.
Good idea! Added.
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 thanks @Ivandemidov00
ps @Ivandemidov00 are you on slack.nats.io dotnet channel?
ouch sorry @Ivandemidov00 I keep forgetting this finally we put a check in github. could you please sign your commits? cheers edit it's a CNCF requirement apparently. thanks again! |
I signed the commits and moved them to the next branch #578. |
Resolves #563