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.

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

ref(rust): Remove strategy.close #361

Merged
merged 2 commits into from
May 15, 2024
Merged

ref(rust): Remove strategy.close #361

merged 2 commits into from
May 15, 2024

Conversation

untitaker
Copy link
Member

I long felt that nobody ever does anything useful in this method in Rust
or in Python. And it makes it a bit harder to think about what "state"
the strategy is in, since strategy.join() sometimes calls close() and
then can no longer be called multiple times.

I long felt that nobody ever does anything useful in this method in Rust
or in Python. And it makes it a bit harder to think about what "state"
the strategy is in,  since strategy.join() sometimes calls close() and
then can no longer be called multiple times.
@untitaker untitaker requested review from a team as code owners May 14, 2024 15:28
@untitaker untitaker requested a review from a team May 14, 2024 15:28
@untitaker
Copy link
Member Author

untitaker commented May 14, 2024

Also strategy.close is incorrectly implemented in rust's RunTaskInThreads. it closes the next strategy before it does its own flushing, which leads to this sequence of events on rebalancing:

  1. StreamProcessor calls self.close
  2. ...calls next_step.close
  3. StreamProcessor calls self.join
  4. ...calls next_step.submit x 100
  5. ...calls next_step.join

according to the API contract, next_step could panic at step 4 because close was already called. luckily nobody ever implemented that in Rust

@untitaker untitaker merged commit bc487fe into main May 15, 2024
11 checks passed
@untitaker untitaker deleted the ref/remove-close-rust branch May 15, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants