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

Fix ordering issue in sequential dispatcher #3947

Closed
wants to merge 13 commits into from

Conversation

rossabaker
Copy link
Member

Passes tests to fix #3945. Needs a bit more refinement, but I understand there's more discussion behind the Discord Curtain about the rewrite being bumped to 3.5. I don't care which solution is taken as long as it gets fixed, but pausing here before more time gets wasted on duplicate effort.

@samspills
Copy link
Contributor

@djspiewak Should we keep working on this fix for 3.5.x? That would give #3923 more time + the 3.6 RC cycle and I think that approach makes sense, since I believe the rewrite still has the not hang when cancelling issue

@djspiewak
Copy link
Member

djspiewak commented Jan 12, 2024

So this effectively retrofits (via convergent evolution) some of the state machine I had to implement for the rewritten Dispatcher but on top of the existing leaning tower that is the current implementation. Absent any other factors, this is reasonable, but holy hell it makes me nervous. I fixed so much stuff (including tests that were specifying incorrect or even contradictory behavior) in the rewrite branch.

Given that there's at least two distinct things that need serious hammock thinking time for 3.6, I'm tempted to say I should rebase #3923 and land it in 3.5.x. I'll try to get to the outstanding issue on it this weekend. Also help very much welcome! I honestly think that the version I have in that PR is a lot easier to build on and fix than the one which is in HEAD.

@rossabaker rossabaker closed this Jan 12, 2024
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.

3 participants