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

Added handshakeType to tell the server about the expected timeout is use. #455

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

Conversation

sgh
Copy link

@sgh sgh commented Dec 16, 2020

For long latency clients then may not be able to deliver the websocket
handshake before we fall back to regular the RFB protocol.

Now it is possible to set expect_ws_handshake=1 which will increase
the timeout to be rfbMaxClientWait or maxClientWait if set.

@matti
Copy link

matti commented Dec 16, 2020

added my comments in the issue #438

@sgh sgh changed the title Added flag to increase time to wait for ws/wss handshake. Added handshakeType to tell the server about the expected timeout is use. Dec 21, 2020
@bk138
Copy link
Member

bk138 commented Apr 28, 2022

This seems to be about configuring the server that only websockets clients will connect and in turn increasing timeout levels. Needs testing and API documentation via doc comments.

@sgh sgh force-pushed the master branch 2 times, most recently from b315c5b to c4711e4 Compare April 29, 2022 06:00
@matti
Copy link

matti commented Apr 29, 2022

increasing handshake wont fix this for slow networks, it just increases propability to succeed and makes ws connections to start slower

this dual mode operation should just be change to be either or

@sgh
Copy link
Author

sgh commented Apr 29, 2022

WS-connections start "immediately" because they transmit the handshake. Regular clients will have to wait - in this case 500ms. The proposed solution will work as long as the network delay is < 500ms. I chose that duration because it seemed fairly acceptable to normal RFB-clients and usually your ping time does not exceed 500 ms. 2G cellular networks might give you 1000 ms roundtrips but I think that VNC will be unusable on 2G. Also - the current 100ms timeout is a "probabillity thing" as well.

The overall protocol design separating RFB and WS/WSS is not optimal - therefore the catch-all solution we be as well :)

@matti
Copy link

matti commented Apr 29, 2022

agreed

Copy link
Contributor

@antenore antenore left a comment

Choose a reason for hiding this comment

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

Beside my 2 comments (1 change), the rest looks good to me.

rfb/rfb.h Show resolved Hide resolved
@@ -74,8 +74,7 @@ Connection: Upgrade\r\n\
Sec-WebSocket-Accept: %s\r\n\
\r\n"

#define WEBSOCKETS_CLIENT_CONNECT_WAIT_MS 100
#define WEBSOCKETS_CLIENT_SEND_WAIT_MS 100
#define WEBSOCKETS_CLIENT_CONNECT_WAIT_MS 500
Copy link
Contributor

Choose a reason for hiding this comment

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

In most use cases 500 ms is more than acceptable, but, generally speaking, we should maybe put all of these constants to a config.h.in file so that a developer could set them to their liking at compile time.

We should probably address this in a future PR, my comment is more to remember.

For long latency clients then may not be able to deliver the websocket
handshake before we fall back to regular the RFB protocol.

So to not introduce a multisecond delay for all conenction a handshakeType
setting is introduced to tell which type of handshake we expect.

The problem is that a RFB client will be quiet "forever" until the server
sends it's signature. On the other hand a websocket client will send it's
HTTP websocket handshake immediately.

Before the server for sure know the the type of client the connect procedure
cannot continue. There is no other option than to wait for some amount of time
when the client type is auto detected.
@bk138
Copy link
Member

bk138 commented Apr 30, 2022

Maybe introducing handshaketype makes things overly complicated: Why not simply use cl->screen->maxClientWait instead of WEBSOCKETS_CLIENT_CONNECT_WAIT_MS or introduce a rfbScreen.maxWebSocketClientWait instead of the harcoded WEBSOCKETS_CLIENT_CONNECT_WAIT_MS?

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

Successfully merging this pull request may close these issues.

4 participants