[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #353: GUACAMOLE-1444: After VNC server network interruption disconnect at client side.

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #353:
URL: https://github.com/apache/guacamole-server/pull/353#discussion_r755536660



##########
File path: src/protocols/vnc/vnc.c
##########
@@ -512,6 +516,14 @@ void* guac_vnc_client_thread(void* data) {
         if (wait_result < 0)
             guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR, "Connection closed.");
 
+        if (server_runing_start - server_runing_end > GUAC_VNC_IDLE_TIMEOUT) {
+         rfb_client = guac_vnc_get_client(client);
+         server_runing_end = guac_timestamp_current();
+            if (!rfb_client) {
+                guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR, "Connection closed.");

Review comment:
       This log message will not be distinguishable from other types of connection closure. If the connection is being closed because the VNC server is not responding, and we can be aware of that cause, then that should be cited in the logs.

##########
File path: src/protocols/vnc/vnc.c
##########
@@ -512,6 +516,14 @@ void* guac_vnc_client_thread(void* data) {
         if (wait_result < 0)
             guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR, "Connection closed.");
 
+        if (server_runing_start - server_runing_end > GUAC_VNC_IDLE_TIMEOUT) {
+         rfb_client = guac_vnc_get_client(client);
+         server_runing_end = guac_timestamp_current();
+            if (!rfb_client) {

Review comment:
       Is this just intended to test whether the connection has already closed?

##########
File path: src/protocols/vnc/client.h
##########
@@ -54,6 +54,11 @@
  */
 #define GUAC_VNC_CLIPBOARD_MAX_LENGTH 262144
 
+/**
+ * The amount of time in milli-seconds to wait before sending a ping to VNC server.
+ */
+#define GUAC_VNC_IDLE_TIMEOUT 10000

Review comment:
       This value should probably be a connection configuration option, not a hard-coded value.

##########
File path: src/protocols/vnc/client.h
##########
@@ -54,6 +54,11 @@
  */
 #define GUAC_VNC_CLIPBOARD_MAX_LENGTH 262144
 
+/**
+ * The amount of time in milli-seconds to wait before sending a ping to VNC server.

Review comment:
       milliseconds*

##########
File path: src/protocols/vnc/vnc.c
##########
@@ -512,6 +516,14 @@ void* guac_vnc_client_thread(void* data) {
         if (wait_result < 0)
             guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR, "Connection closed.");
 
+        if (server_runing_start - server_runing_end > GUAC_VNC_IDLE_TIMEOUT) {
+         rfb_client = guac_vnc_get_client(client);
+         server_runing_end = guac_timestamp_current();

Review comment:
       As Nick mentioned, please correct the indentation such that it matches established style.

##########
File path: src/protocols/vnc/vnc.c
##########
@@ -451,17 +451,21 @@ void* guac_vnc_client_thread(void* data) {
     guac_socket_flush(client->socket);
 
     guac_timestamp last_frame_end = guac_timestamp_current();
+    guac_timestamp server_runing_end = guac_timestamp_current();

Review comment:
       running*

##########
File path: src/protocols/vnc/client.h
##########
@@ -54,6 +54,11 @@
  */
 #define GUAC_VNC_CLIPBOARD_MAX_LENGTH 262144
 
+/**
+ * The amount of time in milli-seconds to wait before sending a ping to VNC server.

Review comment:
       > ... before sending a ping to VNC server.
   
   I'm not sure this is the case. Can you point to where this ping occurs?

##########
File path: src/protocols/vnc/vnc.c
##########
@@ -451,17 +451,21 @@ void* guac_vnc_client_thread(void* data) {
     guac_socket_flush(client->socket);
 
     guac_timestamp last_frame_end = guac_timestamp_current();
+    guac_timestamp server_runing_end = guac_timestamp_current();

Review comment:
       With something like the following:
   
   ```c
   guac_timestamp last_frame_end = guac_timestamp_current();
   guac_timestamp server_runing_end = guac_timestamp_current();
   ```
   
   1. Repeatedly calling `guac_timestamp_current()` will incur unnecessary overhead. Unless there is an expectation that these values will be significantly different, it would be better to just reuse the value from the earlier call.
   2. Can you clarify the difference between the use of these timestamps? Overall, they look very similar to the timestamps that are already being obtained for the frame timing heuristics.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org