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

How to send customized ping message on connectionLostTimeout interval #941

Closed
gharia opened this issue Oct 30, 2019 · 12 comments · Fixed by #944
Closed

How to send customized ping message on connectionLostTimeout interval #941

gharia opened this issue Oct 30, 2019 · 12 comments · Fixed by #944

Comments

@gharia
Copy link

gharia commented Oct 30, 2019

Describe what you would like to know or do
How to send a custom Ping message on connectionLostTimeout?

Describe the solution you'd considered
For creating a WS client, I am extending WebSocketClient. I did setConnectionLostTimeout to 30 seconds. and override the sendPing() method

@Override
  public void sendPing() {
    log.info("Sending ping");
    JsonObject pingObj = new JsonObject();
    pingObj.addProperty("id", "NGenerated guid id");
    pingObj.addProperty("type", "ping");
    this.send(pingObj.toString());
  }

I thought it would be simple, just to override sendPing(). But it is not the case. My overridden method never gets invoked. Rather it calls engine's sendPing()

Additional context
It's not causing any connection close issue, but I am just looking for a way o customize the send ping message.

@marci4
Copy link
Collaborator

marci4 commented Nov 5, 2019

Hello @gharia,

that is currently not possible!

Best regards,
Marcel

@haruntuncay
Copy link
Collaborator

Hello @marci4, I hope you are well and sorry that I wasn't active for a looong time. I would like to start helping again by giving this issue a go, if that's okay ?

@marci4
Copy link
Collaborator

marci4 commented Nov 9, 2019

Hello @haruntuncay,

doing well, hope you are doing good?
Please don't feel obligated. Just look at my activity currently ;)

Of course, feel free :)

Best regards,
Marcel

@haruntuncay
Copy link
Collaborator

haruntuncay commented Nov 9, 2019

@marci4, Can I ask for your advice on something via email ?

@marci4
Copy link
Collaborator

marci4 commented Nov 9, 2019

@haruntuncay always! :)

@haruntuncay
Copy link
Collaborator

haruntuncay commented Nov 17, 2019

Hello @gharia
Could you please clarify whether you want to send a ping frame "with data" or just want to be notified when a ping frame is sent ? The spec allows ping frames to have application data. So in the first case, we can have a way to set ping frame data, but in order to be able to customize the data sent with each ping, more work would be required by the user. For example, we can have another listener method, something like onPingSent for example, and the user would implement it to change the data with each ping.

@gharia
Copy link
Author

gharia commented Nov 18, 2019

Thanks @haruntuncay for looking into it. If possible I would like to send the ping frame "with data" as described in the issue details. ould you point to me an example if any?

@haruntuncay
Copy link
Collaborator

@gharia, Currently, there isn't any way to set ping frame data, but I'm currently giving it a go. I would probably be able to submit a PR for review in 1-2 days.

@haruntuncay
Copy link
Collaborator

haruntuncay commented Nov 18, 2019

@marci4, @gharia, I am uncertain regarding the functionality here. Would support for String data only be enough, or should binary data also be supported ? Also, instead of overwriting sendPing, I was thinking of adding a method called setPingFrameData, since sendPing on WebSocketClient is there just for inheritance, and not actually called.

@gharia
Copy link
Author

gharia commented Nov 19, 2019

@haruntuncay I think support for String data is good enough in case of ping, at least from my perspective. It would be rare if someone wants to send binary data frames along with PING.

Re: sendPing on WebSocketClient - Yes that is not being called on ping event. I am not sure what is the purpose of this method in WebSocketClient, because it always invokes the engine's sendPing()

.

@haruntuncay
Copy link
Collaborator

sendPing is there because both WebSocketImpl and WebSocketClient implements WebSocket interface. But WebSocketClient@sendPing could be made final to avoid this confusion.

@marci4
Copy link
Collaborator

marci4 commented Nov 19, 2019

@haruntuncay I would not limit the data to Strings.
I would simply provide method someone can overwrite, but by default returns new PingFrame().

Maybe extending the WebSocketListener/WebSocketAdapter and providing a default implementation for something like
onPreparePing(WebSocket conn)?

Then sendPing will call onPreparePing() (we have to remove the cached ping frame then).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants