Phoenix: Phoenix.js socket disconnect on a not connected socket

[sb8244]

Environment

• Phoenix.JS version: 1.2.1

Expected behavior

When a disconnect is invoked on a disconnected socket, it should stop trying to reconnect.

Actual behavior

When socket.disconnect() is invoked on a socket which is disconnected (kill phoenix server), it will keep trying to reconnect. If this is combined with creating another socket in certain situations, 2+ sockets can become active when they both reconnect.

Things do work correctly when disconnecting a connected socket.

[TimonVS]

The latest version of Phoenix.JS is 1.3. Have you tried if this version fixes your problem?

[chrismccord]

As Timon said, this has been fixed in the latest release. Please give that a shot and report back if you continue to have issues. Thanks!

[petermm]

Don't think this is resolved - I have the same issue using phoenix.js 1.3, and from a quick look nothing on master that should fix this.

in an offline/server down scenario the reconnectTimer will start running - however if the client does a disconnect the reconnectTimer is NOT reset - and will still be running, and do/attempt a connect..

interim workaround:

realDisconnect() {
  this.reconnectTimer.reset();
  this.disconnect();
}

this might seem edge - but not really - I use it with react native - and when the app is backgrounded a disconnect is called - now if you open the app while offline/server down and close it again - it's left with a running reconnectTimer..
for desktop a logout while offline could do this - or any usage of disconnect while offline..

and worst case is when the stars align and the client will do a connect() unbeknownst to the running reconnectTimer and onConnOpen is called just as this.conn is being set to null by the reconnectTimer - then you have breaking js:

PR/fix - problem is disconnect is called in the reconnectTimer, so you can't simply add reconnectTimer.reset() to the disconnect function, we probably need to add another parameter to the disconnect function

  disconnect(callback, code, reason, reconnectAttempt){
     if(!reconnectAttempt){
       this.reconnectTimer.reset();
     }
    if(this.conn){
      this.conn.onclose = function(){} // noop
      if(code){ this.conn.close(code, reason || "") } else { this.conn.close() }
      this.conn = null
    }
    callback && callback()
  }

and then change the reconnectTimer to

    this.reconnectTimer = new Timer(() => {
      this.disconnect(() => this.connect(), null, null, true);
    }, this.reconnectAfterMs);

but all of this is getting a bit js-ish :/

[chrismccord]

Can you try phoenix.js on master to verify? Thanks!

[josevalim]

@petermm also make sure the compiled .js is changing when you switch phoenix versions because some npm versions had issues updating the generated .js when deps/phoenix changed in disk.

[petermm]

tested on master, and verified that issue still exists. (raw file downloaded, added, and console.logged - so no npm/compile issues)

steps to reproduce:
add console.log to the reconnectTimer:

this.reconnectTimer = new Timer(() => {
      console.log("reconnectTimer");
      this.disconnect(() => this.connect());
    }, this.reconnectAfterMs);

load up simple channel page, that connects to a channel
bring server down - wait to see correct "reconnectTimer" in console
call a disconnect() while server is down
wait and see Unexpected "reconnectTimer" in console.
start server
see Unexpected connect from a "disconnected" client..

[chrismccord]

Thanks for checking. I will take a look!

[sb8244]

Hey @chrismccord. Sorry this dropped off for some reason. I just tested with 1.3.4 and I'm seeing the same behavior.

Specifically, I have socket.disconnect in the socket.onError callback. It then begins trying to reconnect every 1s. Have you ever seen this occur?

[sb8244]

My apologies Chris. My reproduction script (not the original I tested with) had a bug in it. It does appear to be fixed for me!

Thanks for your work 🙏

[sb8244]

I spoke too soon on it being fixed. I just checked out 1.3.4 and noticed that it doesn't contain the fix that is in master. https://github.com/phoenixframework/phoenix/blob/v1.3/assets/js/phoenix.js#L612

I'm quite happy to test out the fix in 1.3.4 as I do have a reproduction step now.

[sb8244]

Some good news. I was able to hackily write your patch in while based on the current 1.3.4 code and it works great:

          this.socket = new this.socketClass(connectionUrl.toString(), { params, reconnectAfterMs }); // eslint-disable-line
          // Invert teardown / disconnect since I can't mess with the Timer callback
          this.socket.disconnect = function disconnect(callback, code, reason) {
            if (this.conn) {
              this.conn.onclose = () => {};
              if (code) {
                this.conn.close(code, reason || '');
              } else {
                this.conn.close();
              }
              this.conn = null;
            }
            return callback && callback();
          }.bind(this.socket);
          this.socket.realDisconnect = function realDisconnect(callback, code, reason) {
            this.reconnectTimer.reset();
            this.disconnect(callback, code, reason);
          }.bind(this.socket);

I then call socket.realDisconnect() to make it disconnect and reset the timer.

[sb8244]

Hey @chrismccord, will this be shipped into an updated Phoenix.js version soon? I checked out the latest on npm, but it looks like the old code still

[chrismccord]

It will be released with 1.4.0 final