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

Duplicate requests from repeated turbo-cable-stream-source signed-stream-name values #559

Open
aprescott opened this issue Jan 17, 2024 · 1 comment

Comments

@aprescott
Copy link

I'm working on an app that uses Turbo 8 through version 2.0.0-beta.2 of turbo-rails.

I've noticed that if a page contains multiple <turbo-cable-stream-source> elements, all with the same signed-stream-name, via multiple calls to turbo_stream_from(foo), refreshes on foo are duplicated, leading to request multiplication for every refresh event.

For example, with 5 copies of the same <turbo-cable-stream-source>, there are 5 subscription requests (shown highlighted below in green), and 1 subscription confirmation (shown below, not highlighted):

Screenshot 2024-01-17 at 17 42 48

(Relatedly, rails/rails#44652 seems to imply that there would be constantly repeated subscription attempts for the N-1 subscriptions? But I don't see that behavior.)

Then, when there is a refresh from an update! on the relevant model (configured with broadcasts_refreshes), the server sends a single websockets message for "<turbo-stream action=\"refresh\"></turbo-stream>" (not 5 of them), and the client initiates 5 requests, 4 of which are canceled:

Screenshot 2024-01-17 at 17 32 53

Although the requests are canceled, the requests may still result in server work, which cumulatively results in a lot of wasteful work, particularly if the endpoints are expensive.

It seems surprising to me that N copies of <turbo-cable-stream-source> all with the same signed-stream-name leads to multiple requests, particularly given that they're auto-cancelled.

Moreover, working around the duplication problem feels like it introduces a lot of complexity: it becomes necessary to track which pages have which subscriptions and for what purpose.

For example, consider a simple model that uses Turbo 8:

class Patient < ApplicationRecord
  broadcasts_refreshes
end

Then suppose there are some related partials:

# _abc.html.erb partial

<%= turbo_stream_from patient %>

<%# ... %>
# _xyz.html.erb partial

<%= turbo_stream_from patient %>

<%# ... %>

In my case, the abc and xyz partials are relatively complex and may appear in different contexts across different pages. The intention is to make sure that using either of these partials results in Turbo-powered updates, hence the embedded turbo_stream_from patient directly into the partials themselves.

However, if a page happens to use both abc and xyz partials, Turbo refresh requests are doubled. With N partials, there are effectively N copies of the subscription.

To avoid that duplication, a developer has to track which pages use abc, or xyz, or both, and then make sure that the relevant turbo_stream_from subscriptions are set up only once, at level of abstraction higher than the partial.

I only just recently started using Turbo, but this behavior feels like it's a bug. It's also interesting that the client is apparently happy with only 1 subscription confirmation from the server, yet that single subscription confirmation still results in N requests. Perhaps that points to the cause of the problem.

@aprescott
Copy link
Author

I have what seems to be a workaround, inspired by rails/rails#44652 (comment).

The idea is to deduplicate consumer.subscriptions.subscriptions by simply dropping the N-1 duplicates subscriptions it contains for each stream name. forget() seems to work just fine (from Action Cable), even though it doesn't notify the server of an unsubscribe.

From the above screenshot of websocket messages, the server in my case only confirms 1 of the N subscription requests, and doesn't send N copies of a given event, but the client processes refresh requests as if all N are connected, leading to N requests. So there doesn't seem to be a need to let the server know the subscription is being removed.

import { cable as ActionCable } from "@hotwired/turbo-rails"

async function deduplicateSubscriptions() {
  const consumer = await ActionCable.getConsumer();
  const subscriptionManager = consumer.subscriptions;
  const subscriptions = subscriptionManager.subscriptions;
  const uniqueSubscriptions = [];

  // Unique-ify the list of subscriptions based on `identifier`.
  subscriptions.forEach((subscription) => {
    const identifier = subscription.identifier;
    const matchingSubscription = uniqueSubscriptions.find((s) => {
      return s.identifier === identifier;
    });

    if (!matchingSubscription) {
      uniqueSubscriptions.push(subscription);
    }
  });

  subscriptions.forEach((subscription) => {
    if (!uniqueSubscriptions.includes(subscription)) {
      subscriptionManager.forget(subscription);
    }
  });
};

const deduplicatingObserver = new MutationObserver((mutationList, _observer) => {
  mutationList.forEach((mutation) => {
    if (mutation.target.tagName === "TURBO-CABLE-STREAM-SOURCE") {
      // `return` so we only dedupe once.
      return deduplicateSubscriptions();
    }
  });
});
deduplicatingObserver.observe(document.body, { attributes: true, childList: true, subtree: true });

Since Object.watch and similar observer APIs are deprecated, I couldn't find a simpler way to deduplicate consumer.subscriptions.subscriptions whenever it's modified. It's a little hacky, but <turbo-cable-stream-source> changes seem to map 1-to-1 to the issue here. More than open to other ideas. It's maybe worth mentioning that the workaround does leave the connected attribute on all N elements, which may be misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant