Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[v2.11] Fix meta step down panic on empty object #5993

Merged
merged 1 commit into from
Oct 11, 2024
Merged

[v2.11] Fix meta step down panic on empty object #5993

merged 1 commit into from
Oct 11, 2024

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Oct 11, 2024

Side effect from #5956 some clients/natscli send empty objects to trigger the meta leader step down.

@wallyqs wallyqs requested a review from a team as a code owner October 11, 2024 20:03
@wallyqs wallyqs changed the title Fix meta step down panic on empty object [v2.11] Fix meta step down panic on empty object Oct 11, 2024
derekcollison
derekcollison reviewed Oct 11, 2024
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jarema will this be an issue for others? Should I hold off on merge?

@derekcollison derekcollison self-requested a review October 11, 2024 21:17
derekcollison
derekcollison approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jarema
Copy link
Member

Jarema commented Oct 11, 2024

This (the isEmptyRequest) protects the API by a side effect.
The actual fix would be to check if pointer is nil before using its fields, as you could still cause panic by sending a field with a typo - not empty, yet Placement would be nil, and cause panic again. At least without strict marshaling.

I will check the other APIs.
This fix is fine as a hotfix, but I would prefer to follow up later if you agree with above @derekcollison .

@derekcollison
Copy link
Member

@Jarema ok to merge this one and we will follow up with separate PR?

Jarema
Jarema approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekcollison derekcollison merged commit 702f5d0 into main Oct 11, 2024
5 checks passed
@derekcollison derekcollison deleted the js-step-down-fix-placement branch October 11, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

@Jarema Jarema Jarema approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wallyqs @Jarema @derekcollison