Skip to content

kvcoord: Close done context if init fails#98268

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
miretskiy:muxrf_old
Mar 13, 2023
Merged

kvcoord: Close done context if init fails#98268
craig[bot] merged 1 commit intocockroachdb:masterfrom
miretskiy:muxrf_old

Conversation

@miretskiy
Copy link
Copy Markdown
Contributor

@miretskiy miretskiy commented Mar 9, 2023 •
edited
Loading

When we close mux rangefeed init context due to an error, it is important to also close client done context so that this error can be propagated to the caller.

Epic: None

Release note: None

@miretskiy miretskiy requested review from a team and erikgrinaker March 9, 2023 00:13
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Mar 9, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Mar 9, 2023

This change is Reviewable

erikgrinaker
erikgrinaker approved these changes Mar 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM. Worth a test case?

@miretskiy
Copy link
Copy Markdown
Contributor Author

miretskiy commented Mar 9, 2023 via email

@miretskiy
Copy link
Copy Markdown
Contributor Author

miretskiy commented Mar 9, 2023

I will likely close this pr

kvcoord: Close done context if init fails
fcb4370 
When we close mux rangefeed init context due to an error,
it is important to also close client done context so that
this error can be propagated to the caller.

Release note: None
@miretskiy
Copy link
Copy Markdown
Contributor Author

miretskiy commented Mar 10, 2023

@erikgrinaker -- I just pushed a change which I think might explain how things got stuck.
The previous version of this change -- I kept; it wasn't needed, but I think it's better to record the actual error, vs
generic context.Canceled. It is also worth noting that this juggling with various contexts caused quite a few issues already -- something that is not relevant in the rewritten mux client implementation.

PTAL.

@miretskiy
Copy link
Copy Markdown
Contributor Author

miretskiy commented Mar 10, 2023

bors r+

@miretskiy
Copy link
Copy Markdown
Contributor Author

miretskiy commented Mar 10, 2023

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 10, 2023

Already running a review

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 10, 2023

Build failed:

@miretskiy
Copy link
Copy Markdown
Contributor Author

miretskiy commented Mar 13, 2023

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 13, 2023

Build succeeded:

@craig craig bot merged commit 9ed78bf into cockroachdb:master Mar 13, 2023
@blathers-crl blathers-crl bot mentioned this pull request Mar 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@erikgrinaker erikgrinaker erikgrinaker approved these changes

Reviewers whose approvals may not affect merge requirements

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

@miretskiy @cockroach-teamcity @erikgrinaker