-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix: discard ReceiveTimeout when timeout message is null #32015 #32016
Conversation
Hi @lolboxen, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
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.
Looks good to me.
It would be best to clean up a small leftover item.
sealed trait Event | ||
case object Ended extends Event | ||
|
||
val interval = 1.second |
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 val seems not to be in use
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.
I think the build will fail because we have a val not in use.
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.
you are correct, somehow I missed then - even after running the validator.
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
References #32015
Approach here is to check if receive message is null and if so stop processing.
Test case may be brittle as it relies on documented behavior but without more advanced knowledge of the project and attempting to test nondeterminism I do not at this time have a better test solution. I welcome suggestions and criticism to make the test case better.