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

[WICKET-6954] implement server side heart-beat + client reconnection in case of inactivity #499

Open
wants to merge 4 commits into
base: wicket-9.x
Choose a base branch
from

Conversation

reiern70
Copy link
Contributor

@reiern70 reiern70 commented Feb 7, 2022

@martin-g

I have tried to use ping-pong approach but I don't know how to implement client side reconnect using it: it seems there is no any "onping" at client side. I have added a second approach were server send some small binary message as "ping" and client side use it as heartbeat to determine server is still there. In case of "disconnection", client will try to reconnect.

There still might be quite a bit of work to do in this PR or maybe use a different approach?

/**
* Boolean used to determine if ping-pong heart beat will be used.
*/
private boolean usePingPongHeartBeat = false;
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference with useHeartBeat ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems ping-pong is designed for the following use case:

-server sends pings to all remote client peers and clients respond with a pong
-server can use this to determine some connection is dead and kill it
-it seems the standard does not include a public onping at client side. Thus browser might respond to this with a pong. Chrome responds with a pong but something like

ws.onping = function() {}
is not called.

Some implementations defines "onping" at client side. See

https://github.com/websockets/ws#how-to-detect-and-close-broken-connections

I have include a class that starts a ping-pong scheduler. But this can't be used a client side in order to reconnect. This uses usePingPongHeartBeat flag.

I have included another timer that send special byte array... This one uses useHeartBeat and is the one used to reconnect broken web-scoket

Not sure if we should keep ping-pong timer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* A {@link IWebSocketMessage message} with Pong message data
*
* @since 6.0
Copy link
Member

Choose a reason for hiding this comment

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

not really

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Copy and waste

*
* @since 6.0
*/
public class PongMessageMessage extends AbstractClientMessage
Copy link
Member

Choose a reason for hiding this comment

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

Why double Message ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tired programmer working during weekends :-)

url += '&wicket-ajax-baseurl=' + encodeURIComponent(WWS.baseUrl);
url += '&wicket-app-name=' + encodeURIComponent(WWS.appName);

console.log(url);
Copy link
Member

Choose a reason for hiding this comment

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

debug leftover

clearTimeout(this.pingTimeout);
// Set a timeout in order to check ping received
this.pingTimeout = setTimeout(() => {
this.ws.close();
Copy link
Member

Choose a reason for hiding this comment

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

I didn't expect to see ws.close() here.
IMO the heartbeat should attempt to send a PING message. If the sending fails or PONG does not come in N millis then try to reconnect M times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no API at server side for this (onPing). I close connection => then recreated it that same way as when page was created => trigger a ConectedMessage with reconnect = true. This I plan to use in our app for delivering things to client what we failed not delivered because connection was gone. This is not a fake scenario. It is happening to us (some progress blocking dialog is staying in page sometimes because connection is broken and events fail to reach client).


@Override
public void terminate(String reason) {
close(-1, "abnormally closed");
Copy link
Member

Choose a reason for hiding this comment

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

setAlive(false)

@@ -79,7 +122,7 @@ public IWebSocketConnection sendMessage(byte[] message, int offset, int length)
protected abstract void onOutMessage(String message);

/**
* A callback method that is called when a text message should be send to the client
* A callback method that is called when a text message should be sent to the client
Copy link
Member

Choose a reason for hiding this comment

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

I think d was correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true... I think IDE just fooled me.


@Override
protected void onOutMessage(byte[] message) {
messageReceived.set(true);
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should call only the old callback method.
This method body should be empty or throw an Exception if it is called by mistake.

{
if (LOG.isDebugEnabled())
{
LOG.debug("Sending unidirectional pon for connection {}", getKey());
Copy link
Member

Choose a reason for hiding this comment

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

pong


import javax.websocket.EndpointConfig;
import javax.websocket.MessageHandler;
import javax.websocket.PongMessage;
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand how JSR 356 spec makes use of javax.websocket.PongMessage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Server pings client --> client implementation answers with a pong --> specification does not force client to offer a onping, which is BAD, because we can't use it for reconnect timeout thing :-(. At least this is what I understood after googling around

@reiern70
Copy link
Contributor Author

reiern70 commented Feb 8, 2022

@martin-g many thanks for your review. I will iterate on PR ASAP

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.

2 participants