panic fix: no version histories is mutable state #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Having no version histories is considered OK by `getMutableState` which
has:
```
if versionHistories != nil {
retResp.VersionHistories = versionHistories.ToInternalType()
}
```
Which means, PollMutableState/GetMutableState should not panic on this.
It is now returning error.
Could make it even better and handle case when initially there were no
versions history, and then appeared, but:
a) an error instead of panic-ing is already better
b) this will complicate tests
c) still not sure how valid this case is (isn't that a corrupted
execution?)
Groxx
reviewed
Jan 2, 2025
| @@ -160,6 +160,15 @@ func (e *historyEngineImpl) getMutableStateOrLongPoll( | |||
| return nil, err | |||
| } | |||
|
|
|||
| if response.GetVersionHistories() == nil { | |||
There was a problem hiding this comment.
🤔 since .GetX() is generally a nil-hiding thing, tbh I think this might be worth changing to response.VersionHistories.
in practice this is fine because:
func (v *GetMutableStateResponse) GetVersionHistories() (o *VersionHistories) {
if v != nil && v.VersionHistories != nil {
return v.VersionHistories
}
return
}but that's sorta abnormal/unexpected for those methods imo. it'd make sense if it was a **VersionHistories, but it's just weird code otherwise (it amounts to if v != nil { return v.VersionHistories } but it's generated code so it's not simplified)
dkrotx
changed the base branch from
cadence-get_mutable_state_or_polling-refactoring
to
master
dkrotx
changed the base branch from
master
to
cadence-get_mutable_state_or_polling-refactoring
davidporter-id-au
approved these changes
Jan 3, 2025
|
Transitioned to cadence-workflow#6589 instead |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Handle error if version histories is nil instead of calling a method (
GetCurrentVersionHistory) on a nil pointer.While returning an empty version histories might look like a good alternative, is not since everywhere in code we expect versions to be not empty. In addition, version histories also has index and it should always be valid (
0refers tohistories[0]).Why?
Because of panics in Uber production.
Having no version histories is considered OK by
getMutableStatewhich has:Which means, PollMutableState/GetMutableState should not panic on this. It is now returning error.
Could make it even better and handle case when initially there were no versions history, and then appeared, but:
a) an error instead of panic-ing is already better b) this will complicate tests
c) still not sure how valid this case is (isn't that a corrupted execution?)
How did you test it?
Added a new unit-test
Potential risks
Release notes
Documentation Changes