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

ActionCable client ensures subscribe command is confirmed. #41581

Merged
merged 2 commits into from Sep 26, 2021
Merged

ActionCable client ensures subscribe command is confirmed. #41581

merged 2 commits into from Sep 26, 2021

Conversation

spinosa
Copy link
Contributor

@spinosa spinosa commented Mar 1, 2021 •
edited by zzak

Summary

A SubscriptionGuarantor maintains a set of pending subscriptions,
resending the subscribe command unless and until the subscription
is confirmed or rejected by the server or cancelled client-side.

A race condition in the ActionCable server - where an unsubscribe
is sent, followed rapidly by a subscribe, but handled in the reverse
order - necessitates this enhancement. Indeed, the subscriptions created
and torn down by Turbo Streams amplifies the existence of this race
condition.

Other Information

The effects of the race condition have also been documented in #38668

Fixes #38668

@rails-bot rails-bot bot added the actioncable label Mar 1, 2021
@spinosa spinosa force-pushed the actioncable_subscription_guarantor branch 2 times, most recently from 3ac6b5c to 9612219 Compare March 2, 2021 18:55
@zzak zzak mentioned this pull request May 28, 2021
Closed
@zzak
Copy link
Member

zzak commented Jun 2, 2021

@spinosa It seems CI is failing, can you take a look? 🙇


{
--
  | "message": "TypeError: _this.subscriptions.subscribe is not a function\nat test/javascript/compiled/test.js:545:11\n\nretrySubscribing/this.retryTimeout</<@test/javascript/compiled/test.js:545:11\nretrySubscribing/this.retryTimeout<@test/javascript/compiled/test.js:543:9\n",
  | "str": "TypeError: _this.subscriptions.subscribe is not a function\nat test/javascript/compiled/test.js:545:11\n\nretrySubscribing/this.retryTimeout</<@test/javascript/compiled/test.js:545:11\nretrySubscribing/this.retryTimeout<@test/javascript/compiled/test.js:543:9\n"
  | }

@spinosa
Copy link
Contributor Author

spinosa commented Jun 2, 2021

@zzak I can, and I will.

FWIW, the code works and I'm open to any comments. I never stood up the Rails test suite and figured I'd come back to the tests if there was any other interest =)

@spinosa spinosa force-pushed the actioncable_subscription_guarantor branch 2 times, most recently from d5ae78d to 30a60f0 Compare June 2, 2021 20:13
@spinosa
Copy link
Contributor Author

spinosa commented Jun 2, 2021

Updated PR with type checking to prevent the errors seen in CI.

Tests pass locally, but they always did. They're not the same as the tests run in CI, where the JS test suite is executed on older browsers via saucelabs.

@spinosa spinosa force-pushed the actioncable_subscription_guarantor branch from 8eebd95 to 30a60f0 Compare June 2, 2021 20:23
@spinosa
Copy link
Contributor Author

spinosa commented Jun 3, 2021 •
edited

@zzak CI still failing, but apparently due to timeouts, and not in actioncable..?

Please advise

@zzak
Copy link
Member

zzak commented Jun 9, 2021

@spinosa Yes, sorry I haven't had time to check on this recently.

Maybe doing a fresh rebase will help clear some of those up?

@spinosa spinosa force-pushed the actioncable_subscription_guarantor branch from 30a60f0 to 2d867a2 Compare June 10, 2021 14:40
@spinosa
Copy link
Contributor Author

spinosa commented Jun 10, 2021

@zzak Good call, buildkite CI passed this go around.

Waiting on the Linters to run w/ approval from maintainer...

@spinosa
Copy link
Contributor Author

spinosa commented Aug 12, 2021

I'm happy to resolve the new conflicts if there's some hope of getting this merged.

@zzak can you help, or point me in a good direction?

Thanks!

@dhh
Copy link
Member

dhh commented Aug 25, 2021

I'd like to see this merged. Please do resolve conflicts and I'll help get this in. Thanks for working on it!

@spinosa
Client ensures subscribe command is confirmed.
b812265 
A SubscriptionGuarantor maintains a set of pending subscriptions,
resending the subscribe command unless and until the subscription
is confirmed or rejected by the server or cancelled client-side.

A race condition in the ActionCable server - where an unsubscribe
is sent, followed rapidly by a subscribe, but handled in the reverse
order - necessitates this enhancement.  Indeed, the subscriptions created
and torn down by Turbo Streams amplifies the existence of this race
condition.
@spinosa spinosa force-pushed the actioncable_subscription_guarantor branch from 2d867a2 to b812265 Compare August 27, 2021 15:57
@spinosa
Copy link
Contributor Author

spinosa commented Aug 27, 2021

@dhh conflicts resolved, buildkite passed, waiting on approval for rest of workflow.

Happy to help! Love this this project and community!

@dhh
Copy link
Member

dhh commented Aug 30, 2021

Awesome, @spinosa. Will get this validated in one of our apps.

@dhh dhh requested a review from jeremy August 30, 2021 12:24
@dhh dhh added this to the 7.0 milestone Sep 10, 2021
@dhh dhh mentioned this pull request Sep 23, 2021
Closed
jeremy
jeremy approved these changes Sep 26, 2021
View reviewed changes
Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Nice work @spinosa!

@jeremy jeremy merged commit 6d7c122 into rails:main Sep 26, 2021
@jeremy
Copy link
Member

jeremy commented Sep 26, 2021

@spinosa Up for doing a backport to 6-1-stable as well?

dhh added a commit that referenced this pull request Sep 30, 2021
@dhh
Merge branch 'main' into support-propshaft
b5e2101 
* main: (78 commits)
  Update MessageEncryptor guide to show that an exception is raised
  Depend on Zeitwerk 2.5.0.beta5
  Fix wrong RDoc
  Add `partial_inserts` default configuration for Rails 7.0 in guides/source/configuring.md
  Improves ActionView FormHelper check_box documentation
  Update .github/pull_request_template.md
  Warning about draft PRs
  Add changelog entry for #42501
  Fix codespell `hiearchy ==> hierarchy` error.
  Replace the word dumb with naive for clarity and inclusivity
  More example in the docs about STI and autoloading
  Client ensures subscribe command is confirmed. (#41581)
  Document easier way to preload STIs
  Revert "Fix ForkTracker on ruby <= 2.5.3"
  Don't override Object#fork in Ruby 3.0
  Fix default scope with all_queries on reload test name
  Use regular memoization
  Avoid class_attrs for unused collection callbacks
  Accept optional transaction args to #with_lock
  Append trailing slash when the path is generated
  ...
@rabbitbike rabbitbike mentioned this pull request Oct 1, 2021
Open
@dhh dhh mentioned this pull request Oct 13, 2021
Closed
@moveson moveson mentioned this pull request Oct 14, 2021
Closed
@spinosa spinosa mentioned this pull request Oct 22, 2021
Merged
@spinosa
Copy link
Contributor Author

spinosa commented Oct 22, 2021

Backport to 6-1-stable:
#43507

@sj26 sj26 mentioned this pull request Mar 10, 2022
Open
@tleish tleish mentioned this pull request Jun 8, 2022
Closed
@palkan palkan mentioned this pull request Jun 30, 2022
Closed
@shoffing shoffing mentioned this pull request Jun 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremy jeremy jeremy approved these changes

Assignees

@jeremy jeremy

Labels
actioncable
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.

ActionCable: Sporadically not receiving confirmation for (re)subscription to same channel
4 participants
@spinosa @zzak @dhh @jeremy