Memory leak: window.onbeforeunload listener added for every new connection

[ansbbrooks]

@jourdain With every new Trame connection made in JavaScript, a window.onbeforeunload event listener is added, that attempts to phone home:

trame-client/js-lib/src/trame.js

Lines 163 to 165 in 9345828

	window.addEventListener("beforeunload", () =>
	this.client?.getRemote()?.Trame?.lifeCycleUpdate("client_exited"),
	);

This leaks an un-disposable Trame object every time the listener is added.

In other words, even if you call trame.exit() and trame.disconnect() on your connection when you're done, and null out all of your references, the Trame object is still lives on in the global scope.

The better way to do this is to send a "heartbeat" signal periodically to the server, to tell the server that the connection is still active. If a heartbeat is not received after a time limit, the server should then consider the client to be de-facto disconnected.

This PR removes the beforeunload listener. I have not investigated on whether a heartbeat is being sent already so I'm not sure if there is also server-side work that needs to be done.

[jourdain]

This is mandatory and related to trame life cyle for user hooks in python.

[jourdain]

• heartbeat: already done at the WebSocket level
• disconnection detection: already done with dedicated timeout to exit server

This life cycle hooks are to allow an application developer to know when a client connects and disconnects.
https://github.com/Kitware/trame-common/blob/main/src/trame_common/decorators/klass.py#L169-L170

So we could include it in trame.exit() and trame.disconnect() and do a better job of clearing that listener in case of disconnect(), but we definitely can not remove that client_exited event.

[ansbbrooks]

This is mandatory and related to trame life cyle for user hooks in python.

The lifecycle user hooks will still work, as long as you are firing the user hooks when one of the following happens:

1. the user explicitly disconnects/disposes the connection
2. the server does not receive the heartbeat after the timeout, in which case it fires the user hook on the client's behalf

[ansbbrooks]

but we definitely can not remove that client_exited event

Oh yes, I am not saying to remove that, I agree with you on that. Just that the onbeforeunload listener should not be there.

[jourdain]

The heartbeat is handle at the transport level and not at the application layer.

This is mandatory and related to trame life cyle for user hooks in python.

The lifecycle user hooks will still work, as long as you are firing the user hooks when one of the following happens:

1. the user explicitly disconnects/disposes the connection
2. the server does not receive the heartbeat after the timeout, in which case it fires the user hook on the client's behalf

Except that right now, none of those will trigger that method call on the server.

[jourdain]

Since this is just for the js-lib, I am ok to remove the onbeforeunload, but the disconnect/exit should be updated to make that call before releasing the client.

[ansbbrooks]

The heartbeat is handle at the transport level and not at the application layer.

This is mandatory and related to trame life cyle for user hooks in python.

The lifecycle user hooks will still work, as long as you are firing the user hooks when one of the following happens:

1. the user explicitly disconnects/disposes the connection
2. the server does not receive the heartbeat after the timeout, in which case it fires the user hook on the client's behalf

Except that right now, none of those will trigger that method call on the server.

The thing is, onbeforeunload is fragile by itself, and shouldn't be relied upon for user hooks. It doesn't fire if the following happens:

browser crash
force quit / kill process
and some more...

[ansbbrooks]

Which one of these close/destroy/disconnect/exit methods should send the client_exited update?

const trame = new Trame();
const client = trame.client!;
const connection = client.getConnection()!;
const session = connection.getSession()!;

await session.close();
connection.destroy();
client.disconnect(disconnectTimeout);
trame.exit(disconnectTimeout);
trame.disconnect();

[jourdain]

It should be done at the trame api class and not in the client.

Add the call in

disconnect() {
    if (this.isConnected()) {
      this.client?.getRemote()?.Trame?.lifeCycleUpdate("client_exited");
      this.client.disconnect(0);
    }
  }

exit(timeout = 60) {
    if (this.isConnected()) {
      this.client?.getRemote()?.Trame?.lifeCycleUpdate("client_exited");
      this.client.disconnect(timeout);
    }
  }

Then your current change will be fine.

[ansbbrooks]

It should be done at the trame api class and not in the client.

Add the call in [...]

Then your current change will be fine.

Done

[jourdain]

Can you?

• bump the version inside package.json
• squash or reword your commits to match semantic-release format
  • chore(js-lib): replace 'beforeunload' by calling lifeCycleUpdate on disconnect/exit call

[ansbbrooks]

Can you?

* bump the version inside package.json

* squash or reword your commits to match semantic-release format
  
  * `chore(js-lib): replace 'beforeunload' by calling lifeCycleUpdate on disconnect/exit call`

Done. Let me know if I got something wrong