You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by gamlerhart <gi...@git.apache.org> on 2017/03/10 01:30:57 UTC
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: Fix Disconnects ...
GitHub user gamlerhart opened a pull request:
https://github.com/apache/incubator-guacamole-server/pull/74
GUACAMOLE-239: Fix Disconnects due to 'backwards' running time.
Fix disconnects due to back ward running time. Fix by using CLOCK_MONOTONIC.
Fix: https://issues.apache.org/jira/browse/GUACAMOLE-239
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gamlerhart/incubator-guacamole-server master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-server/pull/74.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #74
----
commit 68668bec372fb2afa3732f60374b35211a142bf6
Author: Roman Stoffel <ro...@gamlor.info>
Date: 2017-03-10T01:29:02Z
GUACAMOLE-239: Disconnects due to 'backwards' running time. Use CLOCK_MONOTONIC
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-server/pull/74
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by gamlerhart <gi...@git.apache.org>.
Github user gamlerhart commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107279845
--- Diff: src/libguac/timestamp.c ---
@@ -29,13 +31,21 @@
guac_timestamp guac_timestamp_current() {
+#undef CLOCK_MONOTONIC
#ifdef HAVE_CLOCK_GETTIME
struct timespec current;
- /* Get current time */
+ /* Get current time, monotonically increasing.
+ Using monotonic clock when available, to prevent backward running time */
+#ifdef CLOCK_MONOTONIC
+ printf("MONOTONIC USED");
--- End diff --
Fixed...Thanks for your patience to clean up my commits.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by gamlerhart <gi...@git.apache.org>.
Github user gamlerhart commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107795955
--- Diff: src/libguac/timestamp.c ---
@@ -33,9 +33,14 @@ guac_timestamp guac_timestamp_current() {
struct timespec current;
- /* Get current time */
+ /* Get current time, monotonically increasing.
+ Using monotonic clock when available, to prevent backward running time */
--- End diff --
Dropped the second, repeating comment line =)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by gamlerhart <gi...@git.apache.org>.
Github user gamlerhart commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107278773
--- Diff: src/libguac/timestamp.c ---
@@ -29,13 +31,21 @@
guac_timestamp guac_timestamp_current() {
+#undef CLOCK_MONOTONIC
#ifdef HAVE_CLOCK_GETTIME
struct timespec current;
- /* Get current time */
+ /* Get current time, monotonically increasing.
+ Using monotonic clock when available, to prevent backward running time */
+#ifdef CLOCK_MONOTONIC
+ printf("MONOTONIC USED");
--- End diff --
I'm so sorry. Yes...this was part of the testing.
Fixing.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: Fix Disconnects ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107077170
--- 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.
+ When time moves backwards, when last_sent_timestamp can move back,
+ violating the assumption of forward moving time.
+ Ex. Obsorved a Azure Linux instance,
--- End diff --
observed*
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107311953
--- Diff: src/libguac/timestamp.c ---
@@ -33,9 +33,14 @@ guac_timestamp guac_timestamp_current() {
struct timespec current;
- /* Get current time */
+ /* Get current time, monotonically increasing.
+ Using monotonic clock when available, to prevent backward running time */
--- End diff --
The only other issue I see here with respect to wording is:
> Using monotonic clock when available, to prevent backward running time
seems to simply restate that the time is monotonically increasing, which is already stated above... essentially, the whole comment reads to me as: "Get current time, monotonically increasing. Use monotonically increasing time, to ensure time is monotonically increasing."
If you disagree here and feel it adds something, I'm willing to back off and let it be, but it should at least be indented to align with the content of the comment line above it (with leading `*`). I think I may have mentioned this before in prior review, but if not - my apologies. Currently, it's at the same level as a normal line of code, which hurts readability.
Searching through the rest of the guacamole-server codebase, it's rare that we have any comments which span multiple lines, so finding precedent to draw from is tricky, but there are a few examples of how things look in established code, such as:
/* Iterate over all the heat map cells for the area
* and calculate the average framerate */
for (y = min_y; y < max_y; y++) {
(see https://github.com/apache/incubator-guacamole-server/blob/516c4a0593fd78604774bf52c9b547aad5e22619/src/common/surface.c#L395-L396)
Other than that, unless your next revision adds more `printf()` statements, I think we're good to go. ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: Fix Disconnects ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107077159
--- 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.
--- End diff --
monotonically*
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107256748
--- Diff: src/libguac/timestamp.c ---
@@ -29,13 +31,21 @@
guac_timestamp guac_timestamp_current() {
+#undef CLOCK_MONOTONIC
#ifdef HAVE_CLOCK_GETTIME
struct timespec current;
- /* Get current time */
+ /* Get current time, monotonically increasing.
+ Using monotonic clock when available, to prevent backward running time */
+#ifdef CLOCK_MONOTONIC
+ printf("MONOTONIC USED");
--- End diff --
Please remove the debugging statements. Looks like these `printf()`, the `#include <stdio.h>`, and the `#undef CLOCK_MONOTONIC` must have snuck in here during testing. ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: Fix Disconnects ...
Posted by mike-jumper <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: When available, ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107312468
--- Diff: src/libguac/timestamp.c ---
@@ -29,13 +31,21 @@
guac_timestamp guac_timestamp_current() {
+#undef CLOCK_MONOTONIC
#ifdef HAVE_CLOCK_GETTIME
struct timespec current;
- /* Get current time */
+ /* Get current time, monotonically increasing.
+ Using monotonic clock when available, to prevent backward running time */
+#ifdef CLOCK_MONOTONIC
+ printf("MONOTONIC USED");
--- End diff --
No problem. Thanks for your patience in cleaning up your commits.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #74: GUACAMOLE-239: Fix Disconnects ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/74#discussion_r107077427
--- 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.
+ When time moves backwards, when last_sent_timestamp can move back,
+ violating the assumption of forward moving time.
+ Ex. Obsorved a Azure Linux instance,
+ where CLOCK_REALTIME jumped back often, causing disconnects.
+
+ Posix does not require CLOCK_MONOTONIC.
--- End diff --
POSIX*
Why is the indentation of this part of the block comment different from the rest?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---