Fix onClose calls for shared subscribers

[davilima6]

The missing calls to the option-based onClose handlers happen when all shared subscribers that once had the connect flag enabled (thus that had mounted the main useEffect) later change it to false (thus requesting unsubscription).

In that case useEffect is re-run, performing the cleanup for the connect branch, where 2 issues seem to take place:

1. the registration of the onClose handler for each subscriber happened only for the last component unsubscribing for a URL (i.e. only after!hasSubscribers(url) in cleanSubscribers). The order depends on which components un-mounts last, e.g. only a parent's onClose listener would be registered, and later run after socketLike.close() is called.
2. the event register for each subscriber was replacing any existing one, instead of adding to with addEventListener

It can be reproduced either in:

• https://codepen.io/davilima6-the-lessful/pen/xxLQLyy
• https://github.com/davilima6/react-use-websocket-example/

PS: I didn't understand why a complementary mechanism is needed to register the onClose handler, apart from the notifications-based one registered in attach-shared-listeners#bindCloseHandler, like it works for all other listeners.

PS 2: There is no issue related to missing calls when disconnects happen due to server closing.

[robtaussig]

Thank you @davilima6 for taking the time to put together reproducible examples, along with updating unit tests -- I will take a look later this evening. I'm really sorry for the late reply.

[robtaussig]

@davilima6

Ok, so you are absolutely right in your identification of the issue in 1., but I don't think the negative outcome is that the child components (in your example) are not logging the close event, but arguably that anything is logging the close event at all. When a component that is sharing a websocket does one of the following things:

1. Changes the url to which it wants to subscribe;
2. Unmounts; or
3. Willingly disconnects via the connect param

It is ultimately cutting ties with that websocket, and does not care what happens to it in the future (even if that future can be measured in milliseconds). The only reason the websocket closes at all is because there are no subscribers anymore that care what happens to it anymore. Imagine a use-case where your child components all set the connect param to false individually, and only some time later does the main component follow suit -- my opinion is that the onClose callbacks of the child components should not trigger (especially since they might have already re-subscribed to another websocket).

There are definitely some inconsistencies here, however, that I hadn't really thought carefully enough about:

1) When a component with an unshared websocket sets connect to false, its onClose callback is still triggered 2) In this world of micro-seconds, if only the last component that unsubscribes gets the onClose event, why do all components get the onOpen event (you might reasonably expect only the main component to)?

My original justification for 1. was pretty philosophical (and perhaps arbitrary): when a component with an unshared websocket does any of the three actions above, it knows that the websocket will close, and thus more akin to a request to close it than a request to 'unsubscribe' from it. For a component that has only participated as one of (potentially) many subscribers, opting to unsubscribe is less explicitly a request to close (even if in many typical use-cases, if one subscriber disconnects, all are). I'm open to arguments that this is nevertheless confusing and inconsistent -- I have a tendency to rationalize after-the-fact, and usually the longer my explanation, the more uncertain I am.

For 2. this is largely caused by the fact that it is faster for all components to subscribe than it is for the websocket to 'open', and so all subscribers are lined up in time for the open event (even though the open event was initiated immediately after the first subscriber). This is actually the same in the case of closing a websocket: it is faster to unsubscribe than it is for the websocket to completely close, and so it would be awkward to differentiate between a component that is unsubscribing individually (and arguably does not care what happens to the websocket) and one that is unsubscribing at the same time as all other subscribers (and thus is more similar to the unshared websocket).

As for your pull request, I don't think I can approve it (even if I agreed that my current implementation is wrong) -- the way I read it, if:

1) Two components (A and B) are sharing WebSocket 1, and 2) A subscribes to WebSocket 2 (and thus unsubscribes to WebSocket 1), and 3) WebSocket 1 closes (either because B unsubscribes, or because the server closes it), then 4) A's onClose event handler that was added in your pull request will fire for WebSocket 1 and its ReadyState will become ReadyState.CLOSED (even though it is still subscribed to and receiving messages from WebSocket 2).

---

Nevertheless, I'm curious what you (or anyone else) thinks about the current behavior that the last component to unsubscribe has the onClose callback triggered. For the record, I completely agree that by my logic above, this is inconsistent, to say the least. It should not know, nor care, whether it is the last component to unsubscribe. But from a practical perspective, is it important that at least one onClose callback is triggered? If I 'fixed' this by changing:

    if (!hasSubscribers(url)) {
      try {
        const socketLike = sharedWebSockets[url];
        if (socketLike instanceof WebSocket) {
          socketLike.onclose = (event: WebSocketEventMap['close']) => {
            if (optionsRef.current.onClose) {
              optionsRef.current.onClose(event);
            }
            setReadyState(ReadyState.CLOSED);
          };
        }
        socketLike.close();
      }

to

    if (!hasSubscribers(url)) {
      try {
        const socketLike = sharedWebSockets[url];
        if (socketLike instanceof WebSocket) {
          socketLike.onclose = null;
        }
        socketLike.close();
      }

Does that actually benefit any possible use-case? As the last subscriber, there is no risk of the websocket closing unexpectedly in the future, and it's possible that a user is relying on the onClose callback for analytic/logging purposes (and doesn't care which component fired it) -- again, I'm probably rationalizing, so I would love to hear your thoughts on this!

[davilima6]

I appreciate a lot the care you put in your explanations, thank you very much! I will answer properly soon but I agree this PR cannot be merged as-is.

I was looking only at my current use case, a single shared connection/url with multiple subscribing components, and missed the issue of the unexpected "stale" call(s) to onClose when a subscriber is re-connected to another URL. Do you see other problematic cases in this PR? I can try to add tests for those.

[robtaussig]

My only other concern is with using addEventListerner without any mechanism in place to remove the event listener. If a component is subscribing to and unsubscribing from a WebSocket, each unsubscription is creating a new event handler/closure for the close event (you might even have multiple handlers from the same component if it unsubscribes to the same WebSocket multiple times). Also, there is a memory leak risk since by adding these event listeners without knowing whether the websocket is about to close, and whether the component adding them will even exist by the time the event fires, anything captured by the closure will be held up from being garbage collected. While overwriting the onclose does not by itself prevent memory leaks compared to adding an event listener, it significantly limits the scope of the risk because at any given time there will only ever be one 'bad' event handler.

I wonder if there might be another way to solve your use-case? https://stackblitz.com/edit/react-cfpqjj?file=index.js