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

Make time duration since last server data available #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Jul 25, 2023

With my WebSocket connection, I needed the client to re-connect when the connection wasn't active anymore. Setting the heartbeat would ensure the server sent something at a regular interval, but the application had no way to know when the heartbeats were.

This was the solution I came up with, on every received message, store the current performance.now() value, a monotonic millisecond timer. It had to be within this library, because the only message during the time period might be the heartbeat message. Then the application can check if it has been significantly greater than the heartbeat time since the last received message, and attempt to re-connect.

Copy link
Member

@dentarg dentarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like this makes sense, is it possible to cover it with some test?

}

/**
* Get the time since since connection or last data received
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Get the time since since connection or last data received
* Get the time since connection or last data received

@carlhoerberg
Copy link
Member

Shouldn't the heartbeat functionality be built out instead and automatically disconnect if no heartbeat frame is received within the acceptable time frame?

@ngbrown
Copy link
Contributor Author

ngbrown commented Jul 25, 2023

... is it possible to cover it with some test?

Good point, I'll work on a viable test.

Shouldn't the heartbeat functionality be built out instead and automatically disconnect if no heartbeat frame is received within the acceptable time frame?

At the time this was the simplest diff I could patch-in to do what I needed, while detecting disconnects and re-connecting continued to be handled within the app. My application code (in the browser) manages creating and disposing a timer that checks both for closed connections and missed heartbeats. If this timer was moved into the library, a more involved event model between the library and the application would need to be built out.

The .NET client, for example, has an even more involved model of also reconnecting when disconnected, I don't think reconnecting within this library would work easily in my application's case. The application is connecting to RabbitMQ streams, so needs to update x-stream-offset argument to the last value it saw to effectively resume from where it left off. If reconnecting was going to be internalized into the library, then directly supporting streams parameters would be needed.

@carlhoerberg
Copy link
Member

Did you perform any benchmarks with this? How expensive is performance.now()?

The client might not need to reconnect automatically, but socket could be closed and onerror could be called if a missed heartbeat is detected. Then there should be no need to expose this, right?

@ngbrown ngbrown force-pushed the feature/data-received-timestamp branch from 1eaf4be to f38cc02 Compare August 23, 2023 18:25
@dentarg
Copy link
Member

dentarg commented Dec 4, 2023

The client might not need to reconnect automatically, but socket could be closed and onerror could be called if a missed heartbeat is detected. Then there should be no need to expose this, right?

Sounds like that would address #95?

@ngbrown ngbrown force-pushed the feature/data-received-timestamp branch from f38cc02 to c1a20b2 Compare March 15, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants