After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773253 - websocket: add api to add a keepalive interval
websocket: add api to add a keepalive interval
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other All
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
: 773252 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-10-20 07:25 UTC by Ignacio Casal Quinteiro (nacho)
Modified: 2016-11-03 17:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
websocket: add api to add a keepalive interval (6.64 KB, patch)
2016-10-20 07:25 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
websocket: add api to add a keepalive interval (6.72 KB, patch)
2016-10-27 09:30 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
websocket: add api to add a keepalive interval (6.73 KB, patch)
2016-10-27 13:35 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
websocket: add api to add a keepalive interval (6.93 KB, patch)
2016-10-28 11:05 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
websocket: add api to add a keepalive interval (6.93 KB, patch)
2016-10-28 11:11 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review

Description Ignacio Casal Quinteiro (nacho) 2016-10-20 07:25:03 UTC
This will send ping messages in that interval of time so the connection
is kept alive.
Comment 1 Ignacio Casal Quinteiro (nacho) 2016-10-20 07:25:07 UTC
Created attachment 338071 [details] [review]
websocket: add api to add a keepalive interval
Comment 2 Ignacio Casal Quinteiro (nacho) 2016-10-20 07:45:57 UTC
*** Bug 773252 has been marked as a duplicate of this bug. ***
Comment 3 Ignacio Casal Quinteiro (nacho) 2016-10-27 09:30:20 UTC
Created attachment 338590 [details] [review]
websocket: add api to add a keepalive interval

This will send ping messages in that interval of time so the connection
is kept alive.
Comment 4 Dan Winship 2016-10-27 12:09:18 UTC
Comment on attachment 338590 [details] [review]
websocket: add api to add a keepalive interval

>+	 * Interval in seconds on when to send a ping message which will
>+	 * serve as a keepalive message. If set to -1 the keepalive message is
>+	 * disabled.

That immediately led me to ask "what if it's 0?" And it looks like it's disabled in that case too... maybe this should be a uint then, with "0" for disabled?

>+	send_message (self, SOUP_WEBSOCKET_QUEUE_URGENT, 0x09, NULL, 0);

Doesn't necessarily have to be done now, but we should have an enum for the opcodes... (If you do do it now, make it a separate patch.)


Code looks good, but please add a test to tests/websocket-test.c.
Comment 5 Ignacio Casal Quinteiro (nacho) 2016-10-27 13:35:49 UTC
Created attachment 338610 [details] [review]
websocket: add api to add a keepalive interval

This will send ping messages in that interval of time so the connection
is kept alive.
Comment 6 Ignacio Casal Quinteiro (nacho) 2016-10-28 11:05:36 UTC
Created attachment 338681 [details] [review]
websocket: add api to add a keepalive interval

This will send ping messages in that interval of time so the connection
is kept alive.
Comment 7 Ignacio Casal Quinteiro (nacho) 2016-10-28 11:07:09 UTC
This last version fixes 2 problems:
 1. the ping message should be sent with the normal priority otherwise we might endup with mixed messages.
 2. we should stop the ping message timeout right after sending close as the spec says that nothing should be sent after sending close.
Comment 8 Ignacio Casal Quinteiro (nacho) 2016-10-28 11:11:52 UTC
Created attachment 338682 [details] [review]
websocket: add api to add a keepalive interval

This will send ping messages in that interval of time so the connection
is kept alive.
Comment 9 Ignacio Casal Quinteiro (nacho) 2016-11-03 17:07:33 UTC
Thanks for the reviews!

Attachment 338682 [details] pushed as 6fcd8c5 - websocket: add api to add a keepalive interval