[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17090262#comment-17090262 ] ASF subversion and git services commented on PROTON-2030: - Commit b3d1004800b569f69ebea58366021f7ddcd64692 in qpid-proton's branch refs/heads/master from Cliff Jansen [ https://gitbox.apache.org/repos/asf?p=qpid-proton.git;h=b3d1004 ] PROTON-2130: epoll proactor: fix unwritten output bytes, pick up PROTON-2030 and PROTON-2131 > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Assignee: Jiri Daněk >Priority: Major > Fix For: proton-c-0.30.0 > > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17020157#comment-17020157 ] Jiri Daněk commented on PROTON-2030: Yes. There are still some issues as described in comments on PROTON-985, that the bindings use nonmonotonic clocks (Python does, I checked, and I think ruby does too), but the c proactors should be fine now. > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17019713#comment-17019713 ] Andrew Stitcher commented on PROTON-2030: - [~jdanek] is this issue resolved now? > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974293#comment-16974293 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on issue #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#issuecomment-553903765 I'll merge now, better to have it on master and in use, and do more testing later. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974292#comment-16974292 ] ASF subversion and git services commented on PROTON-2030: - Commit aa021087a0de81c99041b325779480041d60e0e8 in qpid-proton's branch refs/heads/master from Jiří Daněk [ https://gitbox.apache.org/repos/asf?p=qpid-proton.git;h=aa02108 ] PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick (#180) - deprecates pn_proactor_now in favour of new pn_proactor_now_64 - replaces use of pn_i_now2 with pn_proactor_now_64 - uses GetTickCount64() in iocp proactor, instead of GetSystemTimeAsFileTime > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974291#comment-16974291 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16968181#comment-16968181 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on issue #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#issuecomment-550207506 I'll try to test this more, either before or after merging. What happens with heartbeating if the system clock wraparounds. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16967634#comment-16967634 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on issue #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#issuecomment-549890751 @jdanekrh I think you should be able to rebase, squash and merge this now. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964140#comment-16964140 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341207935 ## File path: c/src/proactor/win_iocp.c ## @@ -1968,9 +1955,9 @@ class reaper { // Call with lock if (timer_ || !running) return; -pn_timestamp_t now = pn_i_now2(); +int64_t now = pn_proactor_now_64(); pni_zombie_check(iocp_, now); -pn_timestamp_t zd = pni_zombie_deadline(iocp_); +int64_t zd = pni_zombie_deadline(iocp_); if (zd) { DWORD tm = (zd > now) ? zd - now : 1; Review comment: +1 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964136#comment-16964136 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341206698 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: @jdanekrh I'm happy with the change now - I suggest you squash the changes when you rebase the change to merge it though. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964134#comment-16964134 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341206019 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: > The more I think about this, the more confused I feel. I feel your pain. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964126#comment-16964126 ] ASF GitHub Bot commented on PROTON-2030: alanconway commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341202605 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: @astitcher and I are in violent agreement, just like old times. Signed in the API, and the @astitcher unsigned magic in the impl is correct. Speculation about horseshoe crabs out-evolving humans in the distant future is irrelevant (they probably already have) 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964086#comment-16964086 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341169869 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: The more I think about this, the more confused I feel. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964085#comment-16964085 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341169458 ## File path: c/src/proactor/win_iocp.c ## @@ -1968,9 +1955,9 @@ class reaper { // Call with lock if (timer_ || !running) return; -pn_timestamp_t now = pn_i_now2(); +int64_t now = pn_proactor_now_64(); pni_zombie_check(iocp_, now); -pn_timestamp_t zd = pni_zombie_deadline(iocp_); +int64_t zd = pni_zombie_deadline(iocp_); if (zd) { DWORD tm = (zd > now) ? zd - now : 1; Review comment: Here the subtraction will not wraparound, so I am not touching it. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964072#comment-16964072 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341160594 ## File path: c/src/proactor/win_iocp.c ## @@ -3427,11 +3414,9 @@ const pn_netaddr_t *pn_listener_addr(pn_listener_t *l) { } pn_millis_t pn_proactor_now(void) { - FILETIME now; - GetSystemTimeAsFileTime(&now); - ULARGE_INTEGER t; - t.u.HighPart = now.dwHighDateTime; - t.u.LowPart = now.dwLowDateTime; - // Convert to milliseconds and adjust base epoch - return t.QuadPart / 1 - 1164447360; +return (pn_millis_t) pn_proactor_now_64(); +} + +int64_t pn_proactor_now_64(void) { Review comment: Thanks for the explanation. I am going to make the change. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964040#comment-16964040 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341148531 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: To be clear I agree that the external API should maintain signed values - it really has to in any case for back compatibility. but the very specific case of this subtraction needs to be done in correct wraparound mode so unsigned is required here (even if you just cast it back immediately to signed) 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16964039#comment-16964039 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341147231 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: @jdanekrh If it helps the unsigned arithmetic is still correct when the cast signed values are negative. And given that the C language only defines unsigned arithmetic to be correctly wrapping the subtraction must use uints. @alanconway you are assuming that the now value is based at 0 which is not a given - and in fact it is common to test timer wraparound by starting the 'now' monotonic timer at MAXINT-a few minutes. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16963930#comment-16963930 ] ASF GitHub Bot commented on PROTON-2030: alanconway commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341102642 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: Signed arithmetic is a lot easier in general, since you can have negative intermediate results in an expression, and express offsets in both directions in a straightforward way. I'd prefer signed in the public API unless there's a massively good reason to go unsigned. Overflow in 34,000,000,000 years instead of 17,000,000,000 years doesn't strike me as a massively good reason, although obviously programmers in the year 17,000,001,969 will curse my short-sightedness. Trust Andrew to be considerate of the hyper-intelligent horseshoe-crabs that will be masters of the planet then. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16963864#comment-16963864 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341068567 ## File path: c/src/proactor/win_iocp.c ## @@ -3427,11 +3414,9 @@ const pn_netaddr_t *pn_listener_addr(pn_listener_t *l) { } pn_millis_t pn_proactor_now(void) { - FILETIME now; - GetSystemTimeAsFileTime(&now); - ULARGE_INTEGER t; - t.u.HighPart = now.dwHighDateTime; - t.u.LowPart = now.dwLowDateTime; - // Convert to milliseconds and adjust base epoch - return t.QuadPart / 1 - 1164447360; +return (pn_millis_t) pn_proactor_now_64(); +} + +int64_t pn_proactor_now_64(void) { Review comment: @astitcher So you you think that pn_proactor_now whould return signed or unsigned value? Because one response given your comments could be to just go unsigned everywhere. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16963829#comment-16963829 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r341046138 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: Yes, ok. But first I'll spend some time trying to remember why I was so eager on signed arithmetic back then. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16962233#comment-16962233 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r340204445 ## File path: c/src/proactor/epoll.c ## @@ -1426,10 +1419,10 @@ static void pconnection_tick(pconnection_t *pc) { pn_transport_t *t = pc->driver.transport; if (pn_transport_get_idle_timeout(t) || pn_transport_get_remote_idle_timeout(t)) { ptimer_set(&pc->timer, 0); -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { - ptimer_set(&pc->timer, next - now); + ptimer_set(&pc->timer, (uint64_t) next - now); } Review comment: As it is important that the subtraction is done using unsigned arithmetic (to get 'wrap-around' differences correct). So I'd suggest keeping `uint64_t now` & `uint64_t next`. Casting the assignments and leaving the subtraction. It's easier to see this is correct - I can personally never remember which order the conversions are done so even if the arithmetic is correct it takes me a little while to be sure! 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16962234#comment-16962234 ] ASF GitHub Bot commented on PROTON-2030: astitcher commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#discussion_r340205929 ## File path: c/src/proactor/win_iocp.c ## @@ -2145,8 +2132,8 @@ static void pconnection_tick(pconnection_t *pc) { if(!stop_timer(pc->context.proactor->timer_queue, &pc->tick_timer)) { // TODO: handle error } -uint64_t now = pn_i_now2(); -uint64_t next = pn_transport_tick(t, now); +int64_t now = pn_proactor_now_64(); +int64_t next = pn_transport_tick(t, now); if (next) { if (!start_timer(pc->context.proactor->timer_queue, &pc->tick_timer, tick_timer_cb, pc, next - now)) { Review comment: Comment as above in the epoll case - but note that here there may actually be a bug as the subtraction is done using signed int64_t arithmetic which doesn't have defined wraparound semantics. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16962114#comment-16962114 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on issue #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#issuecomment-547477437 Thanks. I'll definitely wait for PROTON-2126 to have AppVeyor green before merging. 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16819950#comment-16819950 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on issue #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180#issuecomment-484030894 I guess i got as far as I can go with this at this point. Anyone could please have a look if this is reasonable? 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16819869#comment-16819869 ] ASF GitHub Bot commented on PROTON-2030: jdanekrh commented on pull request #180: PROTON-2030 Use CLOCK_MONOTONIC in proactors for pn_transport_tick URL: https://github.com/apache/qpid-proton/pull/180 - deprecates pn_proactor_now in favour of new pn_proactor_now_64 - replaces use of pn_i_now2 with pn_proactor_now_64 - uses GetTickCount64() in iocp proactor, instead of GetSystemTimeAsFileTime 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: us...@infra.apache.org > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[jira] [Commented] (PROTON-2030) Use CLOCK_MONOTONIC in proactors for pn_transport_tick
[ https://issues.apache.org/jira/browse/PROTON-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16814211#comment-16814211 ] Jiri Daněk commented on PROTON-2030: I've tried to fix this (I have patch currently in progress) and I've sent e-mail to the mailing list about this. https://www.mail-archive.com/dev@qpid.apache.org/msg79203.html > Use CLOCK_MONOTONIC in proactors for pn_transport_tick > -- > > Key: PROTON-2030 > URL: https://issues.apache.org/jira/browse/PROTON-2030 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-c >Affects Versions: proton-c-0.27.0 >Reporter: Jiri Daněk >Priority: Major > > IOCP and epoll proactors are feeding wall clock time to pn_transport_tick. I > tested (with epoll) that changing system clock breaks heartbeating. > The libuv proactor does use monotonic already. -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org