mike-jumper commented on a change in pull request #228:
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r446698499
##########
File path: src/libguac/protocol.c
##########
@@ -961,6 +975,50 @@ int guac_protocol_send_rect(guac_socket* socket,
}
+/**
+ * Sends the "required" instruction on the given socket, looping
+ * through all the items provided in the NULL-terminated array,
+ * and closing out the instruction. Returns zero on success, or
+ * non-zero on error.
+ *
+ * @param socket
+ * The socket on which to write the instruction.
+ *
+ * @param required
+ * The NULL-terminated array of required parameters to send
+ * to the client.
+ *
+ * @return
+ * Zero if successful, non-zero on error.
+ */
+static int __guac_protocol_send_required(guac_socket* socket,
+ const char** required) {
+
+ // The socket should be kept alive while waiting for user response.
+ guac_socket_require_keep_alive(socket);
Review comment:
Hm ... looking over 9bfcbbf3a576054f82ce9bce6ef2a0497304ca8a, I'm not
sure `guac_socket` allows for this to be done safely by default:
1. The current architecture of `guac_socket` relies on it being impossible
for that `guac_socket` to be used before its handlers have been initialized.
This is because a pointer to the new `guac_socket` simply does not exist
outside the internals of the various `guac_socket` implementations until they
have finished their init process.
If `guac_socket` will now create this thread as part of its own init
(which happens first), then there is no guarantee that the handlers will be
properly initialized by the implementation before the thread starts trying to
write `nop` instructions, leading to unstable behavior.
2. Creating the keep-alive ping on the broadcast socket is nice and
efficient as it only results in a new single thread, with that thread covering
all users of a connection. Creating it across the board would result in a new
thread for the broadcast socket plus new threads for each of the per-user
sockets it writes to, a bit of a waste.
It would be safe for implementations to start the keep-alive ping on their
own, but I don't think `guac_socket` can do this on behalf of all
implementations. There's just no way for those implementations to let
`guac_socket` know that initialization is complete.
My knee-jerk thoughts on the above would be to say that we should start the
keep-alive by default within the broadcast socket implementation and leave it
at that.
What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
