-
Notifications
You must be signed in to change notification settings - Fork 140
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
Heartbeat timeout after update 4.8.1 -> 4.11.1 #255
Comments
Hi there, I will try to have a look on this issue this week. |
Yes, that is correct, first test.each([false, true])('Options#heartbeat, first ping is send after interval', async (shareOption) => {
options.heartbeat = {
message: 'ping',
timeout: 1000,
interval: 500, // <--- interval 500 millis
};
options.share = shareOption;
const {
result,
} = renderHook(() => useWebSocket(URL, options));
if (shareOption) {
renderHook(() => useWebSocket(URL, options));
}
await server.connected;
expect(server).toHaveReceivedMessages([]);
await sleep(100)
expect(server).toHaveReceivedMessages([]);
await sleep(100)
expect(server).toHaveReceivedMessages([]);
await sleep(100)
expect(server).toHaveReceivedMessages([]); // after 300 millis ping is not send
await sleep(200)
expect(server).toHaveReceivedMessages(["ping"]); // but send after 500 millis
server.send('pong')
expect(result.current.readyState).toBe(WebSocket.OPEN); // websocket is still open
}); Are you authenticating your client using WS messages or some other way? Could you please briefly describe your message flow? For example:
Something else that might be important? |
@w666: Sorry for the late reply. The authentication is happening by sending a WS message with a token to the backend in the All of this happens before the first intervall of the hardbeat function is elapsed. A short investigation with Wireshark showed, that the client/frontend is not sending the ping message. Therefore the backend is not sending the reply and this caused the timeout and triggers a reconnect. |
Hi @buhln, Thanks for the detailed info. I will try to write a test to replicate the issue. Will reply here when I have something. |
Check interval between ping messages more often (interval / 10). This is to prevent some configurations that may cause timeouts and connection close. For example, when timeout and interval are set to very close values and there was data message recived right after connection is established. Close robtaussig#255
I was trying to replicate the problem but could not, well, I managed to get a scenario when I got very similar behavior, but one bit is not quite the same. You mentioned that your browser sends token almost immediately after connection is established and reply from server takes just takes a fraction of the second, but looking on your configuration I can assume that your token was sent a bit later. Unfortunately, I don't have enough information to confirm that (no network dump). Anyway, I think the issue is refactored code and your config, I mean this bit
I created a test that has similar config and managed to get behavior that looks like your issue. I decided to improve heartbeat so such configurations should not cause such issues and created a PR. Could you please review it and provide feedback if you agree or not with the design decision. The test in that PR was failing before my changes but pass now. I think it should resolve your issue as well. Thanks. |
I appreciate our effort and I like to test your changes. After installing the package with the changes direct from git my react project is not able to resolve the module longer. It is missing it in the src folder of the project: The entry in the |
There is a workaround how to get changes locally. In you web app project:
In separate temp directory:
Now you can run your app locally in dev mode and you will get patched heartbeat. Hopefully it helps. |
Nice! Sometime the simplest solutions are the best. I tested the changes successful and updated the PR with my results. |
@w666 Thank you for your time and for submitting the PR! I added some comments, but have limited time these days with a baby daughter, so I apologize if I missed anything:) |
I need to refactor code a bit to avoid unnecessary ping messages in shared mode. Was planning to have a look today but there was accident at my neighbor house so lost entire evening. Will try have a look later this week. @robtaussig thanks for taking a look and feedback. |
Thanks. As soon as you are ready I will test it again. |
Check interval between ping messages more often (interval / 10). This is to prevent some configurations that may cause timeouts and connection close. For example, when timeout and interval are set to very close values and there was data message recived right after connection is established. Use one heartbeat interval in shared mode, add test. Close robtaussig#255
Check interval between ping messages more often (interval / 10). This is to prevent some configurations that may cause timeouts and connection close. For example, when timeout and interval are set to very close values and there was data message recived right after connection is established. Use one heartbeat interval in shared mode, add test. Close robtaussig#255
After updating the package on the client side to version 4.11.1, I get all the time a heartbeat timeout. It happens in version 4.10.0, too.
Here my heartbeat config which is working fine in 4.8.1:
On the server side I am running python websockets in version 13.1:
The heartbeat reply is solved as follows:
Downgrading to 4.8.1 removes the issue.
Edit: A short investigation with wireshark showed that in version 4.11.1 the ping message is not send to the backend. Therefore it timeouts. In version 4.8.1 the incoming (from backend side seen) ping text message is shown and the reply from the backend with text message "pong" happens.
On question: My understanding of the source code is, that the timer for the heartbeat is set on
webSocketInstance.onopen
and defined inheartbeat.ts
. Therefore the first heartbeat ping is sent after the defined intervall in the heartbeat-options? I ask this question because when the websocket connection is established to my backend tokens were exchanged to check if the connection is authenticated. If the first message the client sends an heartbeat-ping this message will be dropped.The text was updated successfully, but these errors were encountered: