Use Rails executor instead of reloader to fix Nats::Timeout errors

[zarqman]

When using Rails, this changes the NATS reloader to use the Rails executor instead of the reloader.

This continues to meet the objectives originally outlined in #120.

Rails' reloader is basically a combination of the executor plus hot reloading. Since hot reloading is typically only enabled in development, this should have no impact on production.

In development, when the code has changed, the block wrapped by the Rails reloader is blocked from execution until all other reloader blocks complete. Nested reloaders no-op when inside the same thread. However, NATS connections run in a separate thread.

Most of Rails already runs inside a reloader block, including ActiveJob and ActionController. When a NATS subscription block (in my case, a nats_client.request) is initiated from inside ActiveJob, etc, and the code changes, the subscription's processing block is blocked from execution until the reloader block from ActiveJob, etc finishes. Unfortunately, this means the original NATS request (again, inside ActiveJob, etc) will raise a NATS::IO::Timeout every time, since the replies are indeed blocked from execution by the reloader.

With this PR, only the Rails executor is used to wrap the subscription processing blocks. This continues to isolate database connections (the original concern in #120). And, since the subscription request itself is usually defined in hot-reloadable code, it will be reloaded once that outer block finishes.

If NATS subscriptions are going to be processed in isolated code (outside of Rails' own use of its reloader), and that code wants to enable hot reloading of the subscription block, then the app/daemon running those subscriptions can wrap the subscription in its own Rails reloader block. This is the reason Sidekiq (which was the inspiration for the original use of the reloader) uses reloader where it does.

For example, in #125, the sample bin/nats-listener could enable hot-reloading by wrapping subscribe{ ... } in a reloader. It might also need to register an on-reload handler and, as part of the final loop, know how to unregister/reregister the original subscription. (Whether to wrap subscribe itself, as described here, or just the interior block will depend on the nature of the app/code at hand.)

This change is consistent with the Rails Executor guide which says that libraries calling application code should generally use an executor (section 2.2) and usually only long running process should use the reloader (section 3).

[Envek]

Hey! Author of #120 is here.

With reloader, you're getting these NATS::IO::Timeout only in development, when hot reloading is working, right? (just confirming)

I used Rails reloader specifically to free NATS users from thinking about code reloading in development as, I believe, especially for subscriptions, users tend to define callbacks outside of reloader-wrapped parts of their applications.

For example, in https://github.com/nats-io/nats-pure.rb/pull/125, the sample bin/nats-listener could enable hot-reloading by wrapping subscribe{ ... } in a reloader.

I believe that in that case new subscription will be created on every code reload, no?

[zarqman]

@Envek, thanks for chiming in! Correct, this is only in development with hot reloading.

There are several legitimate uses of subscriptions inside the reloader-wrapped parts of applications. Note that request/response, which is a very powerful part of NATS, uses a subscription internally. Making a request is pretty common inside ActionController or ActiveJob, both of which are reloader-wrapped already.

It's also worth noting that I believe the Rails reloader reloads everything when any watched file changes. So, a change in something as trivial as a view's .erb still causes the NATS::Timeout errors.

To properly support use of subscriptions both inside and outside reloader-wrapped parts of Rails, reloader needs to be changed to executor as presented here. However, any app that wants that reloader to fire can still wrap things in a reloader directly.

I believe that in that case new subscription will be created on every code reload, no?

It depends on where the reloader is added, and what the desired behavior is.

Yes, reloader{ subscribe{ ... } }, which I suggested in the OP, would recycle the subscription every time. And this would also require an extra handler to allow loop{ ... } to cycle. If a gem were to provide a nats-listener-like service, this is probably what it should do, to support the widest array of use cases.

On the other hand, subscribe{ reloader{ ... } } would function the same as you're used to now. As long as that code is not already reloader-wrapped, it's fine. If it is already reloader-wrapped, then it will cause the improper NATS::Timeouts.

Per the Rails Executor guide, the Rails reloader should only be used at a high level. For small code chunks (like a subscription block), only the executor should be used. Certainly it's reasonable for an app developer to choose differently within the context of their app. However, I don't think it's appropriate for a library like nats-pure to deviate from that in a universal way--especially since it has negative side effects like the original NATS::Timeout problem I've been experiencing.