Channels not being closed properly on client side after leave call and termination

[timadevelop]

I've not figured out a solution for this yet, but the following code breaks phoenix.js channel closing logic:

use-phoenix/src/useChannel/useChannel.ts

Lines 172 to 174 in 57e8218

	_channel.off('phx_error');
	_channel.off('phx_close');
	_channel.off('phx_reply');

_channel.off discards all registered callbacks, including the internal one from phoenix:

https://github.com/phoenixframework/phoenix/blob/092605f4c3696c4bfe3a531b024030090c1ea23b/assets/js/phoenix/channel.js#L49-L54

    // phoenix channel.js
    this.onClose(() => {
      this.rejoinTimer.reset()
      if(this.socket.hasLogger()) this.socket.log("channel", `close ${this.topic} ${this.joinRef()}`)
      this.state = CHANNEL_STATES.closed
      this.socket.remove(this)
    })

Imagine you have a channel that should be joined and left upon chat open/close in UI. If we open & close & open the chat, it should create channel 1, close channel 1, create channel 2 with the same topic (not 1 because 1 should be closed at this point).
The callback above is discarded before phoenix has a chance to call it after channel process termination on server, which causes the closed channel to stay in socket.channels with leaving state instead of being removed from there. This makes findChannel being able to find this "zombie" channel instead of creating a new one -> we attach to "zombie" channel in handleJoin instead of creating a new one.

use-phoenix/src/useChannel/useChannel.ts

Lines 90 to 91 in 57e8218

	const existingChannel = findChannel(socket, topic);
	if (existingChannel) return handleJoin(existingChannel);

---

I see the reasoning behind these off calls, I don't have the exact repro for the described use case below, but perhaps we could use channel.off(event, ref) instead of channel.off(event) to fix the issue with "error"?

use-phoenix/src/useChannel/useChannel.ts

Lines 144 to 174 in 57e8218

	/*
	So the problem is that these .recieve() functions stay persisted even after this hook
	has potentially moved on to an entirely different topic.
	So consider the following scenario:
	- we connect on topic 'room:1' and we error. That is, we aren't permitted to enter.
	- Then, using the same hook, we connect to room:2 and we are permitted.
	- Since the first one errored, **and the socket has configured rejoin() timeouts**,
	the socket will attempt to rejoin 'room:1' even though we have already moved
	on to 'room:2'. The rejoin attempt will actually call the recieve functions
	and be able to update the state of the hook, even though we are no longer
	interested in that topic.
	- So, we will be in a success state after joining 'room:2', and then rejoin will
	trigger and the recieve('ok') will be called, and since it's for room:1, it will
	update the state of the hook back into an error state.
	- Here, once the topic changes, we remove all recieve hooks to prevent this from happening.
	- So the rejoin() attempt will be called, but this hook won't be listening!
	This can be entirely avoided if, the hook-user correctly calls leave() when the `room:1` join
	fails, but that's not a guarantee, and I think the expected behavior is that the hook no
	longer cares about it's previous topic.
	*/
	// @ts-ignore
	if (_channel.joinPush) _channel.joinPush.recHooks = [];
	_channel.off('phx_error');
	_channel.off('phx_close');
	_channel.off('phx_reply');

Tbh, I believe the solution with .off/1 or .off/2 on errored channels is not robust and might lead to other issues. I do not have good ideas now, but perhaps it'd be better to find a reasonable way to leave / remove these channels from socket.channels on client side instead of allowing them to stay there forever. I understand that the lib cannot decide if client actually wants this behavior, so maybe it's indeed the job of the client to correctly call leave?

[Jdyn]

Hey thanks for the write up. With the current logic, I'm able to join -> leave -> join a channel, and I don't see the previous channel in with state: 'leaving' inside socket.channels.

For reproing, the easiest way is to have some channel like room:id and then connect on the frontend with a bad id, such that the server responds with {:error, message}. Then without calling leave on the frontend, connect to room:id with a good id.

After a moment, you will see a join attempt on that previous bad id and the hook state should update to it's error state since it recieved the error again from that first channel.

I think there is a more elegant solution for sure, but this did seem to work and fit into the "react" way of doing things, where the topic is driving the state of the hook.

If it's causing trouble, im open to removing the logic and then putting it on the dev to leave the channel. Although, if a dev wrote code where the topic in a mounted useChannel changes, I feel like it's clear they want this channel to leave the previous topic. What are your thoughts?