Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107078078
--- Diff: src/libguac/timestamp.c ---
@@ -33,9 +33,21 @@ guac_timestamp guac_timestamp_current() {
struct timespec current;
- /* Get current time */
+ /* Get current time, monotinically increasing.
+ guacd expects that a client sync timestamp <= last_sent_timestamp.
--- End diff --
Perhaps leaving this whole thing simply as:
/* Get current time, monotonically increasing */
would be better?
While that is indeed enforced, and violation of that rule results in the
connection being summarily closed, the definition of `guac_timestamp_current()`
already states:
> The difference between return values of any two calls is equal to the
amount of time in milliseconds between those calls.
which implicitly requires that the time be monotonically increasing. This
function isn't supposed to care about the internals of the Guacamole protocol,
guacd, libguac, etc.; only timestamps. It's good to document this within the
JIRA issue related to the change (which I believe you've already done), but
documenting external behavior here violates separation of concerns IMHO, and
the function definition itself is already enough to require this change.
ie: The issue is not that guacd expects timestamps will be monotonically
increasing, but rather that it (and any other user of this function) is
supposed to be able to rely on that fact.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
