Global config, simplify threading, custom operations

[hopsoft]

• Move configuration to main namespace
• Use process wide singleton for config
• Use thread singleton for Channels
• Eliminates the cyclical dependency between Channel and Channels
• Tightens security around broadcast contexts
• Addresses multi-threaded broadcast race conditions
• ⚠️ Will likely force a major version bump

CableReady::Config.instance   # returns a singleton (state shared by all threads)
CableReady::Channels.instance # returns a thread local singleton (no shared state)

Custom operations can be added like so (at any time of the application lifecycle):

CableReady.configure do |config|
  config.add_operation_name :custom_magic
end

See the full example of how to use custom operations.

Note that StimulusReflex users will need to wait for stimulusreflex/stimulus_reflex#421 before they can use custom operations via the cable_ready helper method in their reflexes.

[leastbad]

I see that we're moving configuration to the main namespace, which seems like a very reasonable thing to do. I don't love that we're changing add_operation to add_operation_method. I call them operations throughout the documentation and the code itself.

https://cableready.stimulusreflex.com/customization#custom-operations

If you're really hung up on it, might I suggest an alias as a compromise?

[leastbad]

So, having spent time with this, it seems like a great refactor. I've got some procedural questions that I want to ask for completeness:

What, if any changes will be required to work with existing applications? Are the breaking changes confined to the configuration object + defining custom operations, or are there more changes that will impact more developers?

Aside from the configuration stuff, has the public API changed at all?

For example, I noticed that you added chaining to add_operation_method which I think is cool. But you also added it to broadcast and broadcast_to which I don't totally know if I buy into it. I kind of like the idea of a chain that terminates, but maybe I'm just being closed minded. Did you have a specific use case scenario in mind for the never-ending chain? Unless you can somehow insert a delay into the chain, I have a hard time understanding what this really buys you.

Finally, how does thread-local help with the security concerns? I read through that project's README but since users aren't assigned to a specific thread (are they!?) it seems like this is a partial solution but you're still on a party line. Happy to learn otherwise!

Anyhow, I just ran my CableReady test suite with this PR and after I did a search-and-replace for cable_ready. to cable_ready[stream_name]. everything seems to work like a charm. I'm going to try stimulusreflex/stimulus_reflex#421 next.

[hopsoft]

I don't love that we're changing add_operation to add_operation_method

I'm being explicit and pedantic for clarity and to avoid future confusion. Note also that add_operation_method should be considered internal interface and not called directly by library users. Consider the following definitions.

• CableReady.config.add_operation_name - indicates that you'd like CR to support an operation of this name/type
• CableReady::Channel#add_operation_method - adds a new method to the channel instance (a thread based singleton) for the given operation name. The only reason this method is public is because the config observer invokes it whenever new operation names get added, but this method should not be called directly by consumers of the library.

What, if any changes will be required to work with existing applications?

The registering of custom operations.

Aside from the configuration stuff, has the public API changed at all?

No

Did you have a specific use case scenario in mind for the never-ending chain? Unless you can somehow insert a delay into the chain, I have a hard time understanding what this really buys you.

True. I was thinking of scenarios like:

cable_ready[...]
  .dispatch_event(...).broadcast # should execute on the client first
  .inner_html(...).broadcast

But, given that broadcast simply hands things off to ActionCable internals, it's possible that a race condition would still prevent guaranteed ordered delivery of the payloads. I'm good if we want to omit chaining on broadcasts to help avoid support tickets on this.

how does thread-local help with the security concerns? ...users aren't assigned to a specific thread (are they!?)

CR broadcast payloads will be discreet and isolated to the thread that's handling the web request, background job, or web socket message. ActionCable, Puma, and Sidekiq all use a thread pool for performing work. Once a thread starts working on behalf of a user, it generally remains dedicated to that user until the work is complete. Sophisticated use of fibers can muddy the waters a bit; however, we're much further ahead by isolating CR channels and operations to thread based singletons.

[leastbad]

Thanks for all of that. I still would prefer add_operation over add_operation_name since we are adding an operation and not an operation_name (so how is this clearer?) but it's not the hill I'm going to die on. 👻

The concern I have with pushing a major update is that I assume there will be growing pains with either bundler or yarn when we bump SR to require v5 of CR. If people have explicitly required v4.x.x in the Gemfile/package.json, will there be warnings or will it just do it anyhow?

[hopsoft]

I think we could avoid a major version bump if we have a good handle on the folks currently using custom operations. We could work with these folks to ensure a seamless upgrade.

[leastbad]

@n-rodriguez and @jonsgreen (and me) so far as I know.

[leastbad]

It's only been a little while, but it seems like only Nicolas is using them seriously in production so far. Other than the docs, we haven't seen any blog posts or anything about it, so this probably isn't shocking. Nicolas, you're a trailblazer! 😀

[n-rodriguez]

It's only been a little while, but it seems like only Nicolas is using them seriously in production so far. Other than the docs, we haven't seen any blog posts or anything about it, so this probably isn't shocking. Nicolas, you're a trailblazer! grinning

😂

[leastbad]

Alright, @hopsoft, so here's what's shaken out for me.

We'll change add_operation to add_operation_name because you have your reasons. 😛 I'll update the docs.

I do think we should omit chaining from broadcast and broadcast_to.

Given the lack of response, I think we can safely update our customization API without a major version bump as we're in conversation with everyone using it.

If you get rid of those two self statements, I think we can merge this. I did have some questions on stimulusreflex/stimulus_reflex#421 (comment). I think hopsoft/stimulus_reflex_expo#67 is ready to go once this is in - I changed config to operation there as per my note.

[n-rodriguez]

It works perfectly fine! 👍