Skip to content

Conversation

@jameshilliard
Copy link
Contributor

Alternative to #415.

@ar
Copy link
Member

ar commented May 28, 2021

You mix stuff in the PRs which make them difficult to accept.

Can you reference which timeout calculation bug you are referring too?

@jameshilliard
Copy link
Contributor Author

You mix stuff in the PRs which make them difficult to accept.

The other changes are somewhat related to the primary fix.

Can you reference which timeout calculation bug you are referring too?

The main issue is with this code:

timeout = maxWait - System.currentTimeMillis();
if (timeout >= 0)
return mux.request (m, timeout);

Where the if (timeout >= 0) check is done against a recomputed timeout instead of the timeout originally passed to the function. The recomputed timeout additionally appears to result in inconsistent behavior when exactly 0 since QMUX requests appear to have special cased behavior for timeouts of zero here(and in a few other places I think):

if (timeout > 0)
sp.out (out, m, timeout);
else
sp.out (out, m);

@ar
Copy link
Member

ar commented May 28, 2021

I understand you are referring to issue #414 - we don't need nano precision and excessive object reaction to fix that.

In addition, your fix doesn't address the real intent of using a 0 timeout, which is fire-and-forget the message. In addition, this fix only fixes the synchronous call to MUX.request, not the async one.

I addressed this issue in 4cd6c95 - we can raise a case for the monotonic clock stuff separately.

@ar ar closed this May 28, 2021
@jameshilliard
Copy link
Contributor Author

I understand you are referring to issue #414 - we don't need nano precision

Reason for using System.nanoTime() isn't precision related but rather due to System.currentTimeMillis() being unsafe for elapsed time calculations due to using the wrong type of clock. Note on Linux System.nanoTime() calls are actually faster than System.currentTimeMillis() calls.

excessive object reaction to fix that.

There is some overhead from this but it seems to be minimal from some benchmarks I did, we could technically avoid it but not sure that would be worth it.

@jameshilliard
Copy link
Contributor Author

In addition, your fix doesn't address the real intent of using a 0 timeout, which is fire-and-forget the message. In addition, this fix only fixes the synchronous call to MUX.request, not the async one.

Hmm, the async version still doesn't really look right to me. Reworked some of the logic here more in #425.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants