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

only run piggybacked operations after SR is finished, and isolate them #482

Merged
merged 11 commits into from
Apr 28, 2021

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Mar 29, 2021

Type of PR (feature, enhancement, bug fix, etc.)

Bug fix

Description

Piggybacked operations would run for all tabs, which is a problem for isolation mode. Also, there could be a race condition where piggybacked operations might start running before SR operations are completed.

This PR fixes both issues.

Post-script

This ended up being a lot more involved than I originally anticipated. The primary change ended up requiring that I modify the method_missing in cable_ready_channels.rb so that the reflex_id is added to the options for every operation. The client then compares the reflexId attached to the operation against the reflexes dictionary to decide whether it should be executed on the current tab context. It seems like a viable solution but there could be cases I haven't considered. Would appreciate more brains on this.

I've spent the last few hours attempting to isolate and address a 2nd race condition that emerged when sending CR broadcasts inside a Reflex. It seems like the path to CableReady.perform(data.operations) on line 105 of reflexes.js is super short, right? Not only is the WebSocket message with the CR broadcast sent before the morph broadcast, but the reflexOperations on line 101 kicks off a multi-stage process involving several events and potentially DOM updates... and yet! somehow, the question of whether line 101 or 105 fires first is non-deterministic. I'm at a complete loss to explain this phenomena. Apparently Object.entries (line 104) is slow, but it was happening even when I commented out line 104... and come on, it's not that slow.

Still, you can't argue with observable, repeatable observations. If I wrap line 101 in a setTimeout(,5) the problem goes away about 99% of the time, although I still saw 1-2 show up out of order. As of right now, I've taken this as far as I can.

What I plan to tell people is that if the order of operations is critcal, eg. that it must be

  1. CableReady broadcasts
  2. Morphs
  3. CableReady operations that piggyback on the morphs

... then they should follow their cable_ready calls with a sleep 0.01 to sleep 0.1 so that the CableReady broadcasts have a good chance to finish before the morph returns.

In other words, this will work:

def test
  cable_ready.console_log(message: "am i before the morph, or after?").broadcast
  morph :nothing
end

but if you need to guarantee that the console_log executes before the morph returns, you're looking at:

def test
  cable_ready.console_log(message: "definitely before the morph").broadcast
  sleep 0.01
  morph :nothing
end

In order to make this work reliably without sleep calls (although still non-deterministically ordered) I had to go ahead and comment out (former) line 74 of lifecycle.js which removed completed reflexes from the reflexes dictionary. This is something I already planned to do in the next major version of the library, and there's no obvious downside although long-running applications might start to occupy memory if lots of Reflexes are used. (The plan is for the next major version to have a reaping strategy, sort of like how Redis has a key expiration strategy. Hopefully, this will be fine without reaping until the next version is ready.)

Final thing: in channel.rb on line 100 I added a safe navigation operator to the Reflex logger print statement. This means developers will be able to suppress server-side logging for a single Reflex if their Reflex action method contains @logger = nil.

Why should this be added

Race issues are bad.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@leastbad leastbad added this to the 3.4.2 milestone Mar 29, 2021
@leastbad leastbad added bug Something isn't working javascript Pull requests that update Javascript code labels Mar 29, 2021
@leastbad leastbad merged commit 5b71b16 into stimulusreflex:master Apr 28, 2021
@leastbad leastbad deleted the piggyback branch August 16, 2021 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant