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

Incorrect closing of the client when the channel destroying #349

Open
MarhiievHE opened this issue Sep 9, 2022 · 2 comments
Open

Incorrect closing of the client when the channel destroying #349

MarhiievHE opened this issue Sep 9, 2022 · 2 comments
Labels
bug Something isn't working

Comments

@MarhiievHE
Copy link
Member

Describe the bug

This issue occurs when using subscriptions. In some cases, the client instance does not close correctly when the channel is destroyed, and the close event does not happen. As a result, when trying to emit something to this client instance, we get an error, since there is no such key in the channels map.

To Reproduce

To confirm the problem, the destroy method in the Chanel class and the emit method in the Client class were modified:

  destroy() {
    for (const callback of this.client.events.close) callback();
    channels.set(this.client, null);
    if (!this.session) return;
    sessions.delete(this.session.token);
  }
  emit(name, data) {
    const packet = { event: --this.eventId, [name]: data };
    const channel = channels.get(this);
    if (channel === null) {
      throw new Error('Emmiting on a destroyed client');
    }
    if (!channel || !channel.connection) {
      console.log(channel, this, [...channels]);
      throw new Error(`Can't send metacom events to http transport`);
    }
    channel.send(packet);
}

Screenshots

image

Desktop (please complete the following information):

  • OS: [alpine3.15 in docker]
  • Node.js version [v16.17.0]
@MarhiievHE MarhiievHE added the bug Something isn't working label Sep 9, 2022
@nechaido
Copy link
Member

nechaido commented Sep 9, 2022

This happens due to the fact that when the channel is destroyed, no close event is emitted on the client, as we expect the other side to reconnect to the same client IIUC. Possible solutions:

  1. Add intermediary reconnecting event and status to the client and check it when sending the event. We would need a proper reconnection strategy and event buffer for this. So we do not lose any data.
  2. Treat this situation as an exception so that users can handle it themselves. Add NULL-ish connection (Symbol.for('DESTROYED-CONNECTION')) and serve it to channels Map on connection.destroy(). We would still need to collect it garbage somehow.
  3. Leave it as it is, but change the error message to Channel can't send events. And add the clarification to the documentation that this error will occur if channel` is not ready or does not support events.
  4. Have 2 distinct errors Channel is destroyed (if !channel) and Channel does not support events ( if !channel.connection)

@MarhiievHE
Copy link
Member Author

MarhiievHE commented Sep 10, 2022

I slightly changed the error catching code, and here is the result:

12:54:05  W6   log     destroy chanel { idClient: 193n, accountId: 4577, closeCount: 2 }
12:54:29  W6   log     Emmiting on a destroyed client { idClient: 193n, accountId: 4577, closeCount: 4 }

Changed code

Client

class Client {
  static idClientN = 0n;
  constructor() {
    this.events = { close: [] };
    this.eventId = 0;
    this.streams = new Map();
    this.streamId = 0;
    this.idClient = Client.idClientN++;
  }
...
 emit(name, data) {
    const packet = { event: --this.eventId, [name]: data };
    const channel = channels.get(this);
    if (channel === null) {
      const { idClient, accountId } = this;
      const closeCount = this.events.close.length;
      console.log('Emmiting on a destroyed client', {idClient, accountId, closeCount});
      for (const callback of this.events.close) callback();
      //throw new Error('Emmiting on a destroyed client');
      return;
    }
    if (!channel || !channel.connection) {
      console.log(channel, this, [...channels]);
      throw new Error(`Can't send metacom events to http transport`);
    }
    channel.send(packet);
  }
...
}

Chanel

  destroy() {
    const { idClient, accountId } = this.client;
    const closeCount = this.client.events.close.length;
    console.log('destroy chanel', {idClient, accountId, closeCount});
    for (const callback of this.client.events.close) callback();
    channels.set(this.client, null);
    if (!this.session) return;
    sessions.delete(this.session.token);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants