Fix exception in ActionCable channel due to reflexes overwriting each other's data

[alexander-makarenko]

Bug fix + refactoring

Description

Think of a page with multiple StimulusReflex controllers hooked up to the stimulus-reflex:connected event and the markup (Haml in this example) looking something like this:

%div{ data: { controller: "sandwich", action: "stimulus-reflex:connected@document->sandwich#eat" } }

%div{ data: { controller: "tea", action: "stimulus-reflex:connected@document->tea#drink" } }

The actions are backed by simple JavaScript controllers and reflex actions looking as follows. The reflex actions have different arity: one take no arguments, the other takes one.

// sandwich.js:

eat() {
  this.stimulate("Sandwich#eat");
}

// tea.js:

drink() {
  this.stimulate("Tea#drink", "green");
}

# sandwich_reflex.rb:

class SandwichReflex < ApplicationReflex
  def eat
    # ...
  end
end

# tea_reflex.rb:

class TeaReflex < ApplicationReflex
  def drink(flavour)
    # ...
  end
end

This kind of setup creates a situation where StimulusReflex::Channel#receive is invoked twice in a very short period of time. And since both messages are processed by the same instance of the ActionCable channel (as can be verified by printing out self.object_id at the beginning of the method), the use of an instance variable (@reflex_data) in the current implementation leads to a situation where it gets overrwritten by the second message before the first one has been processed. As a result, reflex_data inside #delegate_call_to_reflex ends up containing the same set of data which, after being checked against the method signature of both of the reflex actions, raises an ArgumentError due to arity mismatch.

This pull request fixes the issue by eliminating the use of instance variables inside StimulusReflex::Channel#receive in favour of bundling the message payload with the respective instance of the reflex. It also simplifies the reflex factory and constructor by delegating most of the calls to the StimulusReflex::ReflexData instance.

Why should this be added

In modern web applications, it is common to load parts of the page asynchronously for a more responsive user experience. With StimulusReflex, it is most easily achieved by hooking up to the stimulus-reflex:connected event. This pull request fixes an exception that prevented the use of the specified event with multiple reflexes.

Checklist

• My code follows the style guidelines of this project
• Checks (StandardRB & Prettier-Standard) are passing

[netlify]

✅ Deploy Preview for stimulusreflex ready!

Name	Link
🔨 Latest commit	20c0a7a
🔍 Latest deploy log	https://app.netlify.com/sites/stimulusreflex/deploys/64e8d558e82cd80007fe54b9
😎 Deploy Preview	https://deploy-preview-663--stimulusreflex.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[Matt-Yorkley]

Nice refactor ❤️

[alexander-makarenko]

@Matt-Yorkley Thanks for your review. I have addressed your comments and also fixed the tests and linter warnings. Would appreciate if you could give it another look when you have time.

[alexander-makarenko]

@Matt-Yorkley Thanks for your suggestions! I have addressed all feedback.

[alexander-makarenko]

@Matt-Yorkley How can we proceed? Is there anything preventing this PR from getting merged?

[marcoroth]

Hey @alexander-makarenko, thanks so much for working on this!

Do you have an app or a test case which we could use to reproduce the issue itself? Just to make sure we are not re-introducing this by accident again in the future, even if it's a manual test.

I just want to make sure we are not breaking other things with this change, as there are quite a few changes here.

[alexander-makarenko]

@marcoroth Since it's a race condition, it's actually quite difficult to reproduce, since it depends on how fast the StimulusReflex code runs in a specific application on a specific system. In a larger production application where I originally discovered the issue, it failed about once every 50 page loads. In the scenario that I outlined in the description of this PR (with no logic in the JS controllers or reflex), it seems to fail even rarer on my computer.

In order to debug the issue, I remember that I cloned the repo of StimulusReflex 3.5.0.pre9 locally and pointed my application to use it. Then I added a sleep somewhere down StimulusReflex::Channel#receive to emulate slow processing of messages, until I was able to reproduce the issue consistently. I then fixed the implementation of said method to eliminate the issue and submitted my changes in this pull request.

Thus, due to the nature of the issue, I unfortunately won't be able to provide a test application or add an automated test for it.

[RolandStuder]

Absolutely great catch, I really think think this should be merged, the bug you uncovered is the kind that can haunt people for weeks if not months, we should prevent such pains!

I did not make a thorough review, but noticed one line removal, that may reintroduce an existing but. I don't think it's covered by a test. See my comment

[marcoroth]

Thank you so much for this @alexander-makarenko!