Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wait_until fails with "Invalid argument" when called too soon after reboot #308

Open
maflcko opened this issue Feb 29, 2020 · 6 comments
Open

Comments

@maflcko
Copy link

maflcko commented Feb 29, 2020

wait_until accepts a time point that may be in the past. This generally works fine unless the time point is before the machine booted up.

The boost implementation will first calculate the difference to the current time: https://github.com/boostorg/thread/blob/boost-1.70.0/include/boost/thread/pthread/condition_variable_fwd.hpp#L270

Right afterward, the difference is added to the internal clock (detail::internal_chrono_clock::now() + d). However, the internal clock only starts when the machine was booted up (and not when the unix epoch started). Thus, the resulting timeout that is passed to do_wait_until may be negative (in both values, seconds and nanoseconds). Then pthread_cond_timedwait throws "Invalid argument".

@maflcko
Copy link
Author

maflcko commented Mar 2, 2020

Boost 1.66 works fine, and Boost 1.67 is the first version that is broken.

#167 landed in 1.67, so it might be related.

@maflcko
Copy link
Author

maflcko commented Mar 2, 2020

@maflcko
Copy link
Author

maflcko commented Mar 2, 2020

I wrote a test in bitcoin/bitcoin#18232 to illustrate the issue

@fanquake
Copy link

fanquake commented Mar 2, 2020

Boost 1.66 works fine, and Boost 1.76 is the first version that is broken.

Do you mean Boost 1.72.0 here (latest release)? So the broken range is 1.66.0 to 1.72.0?

@maflcko
Copy link
Author

maflcko commented Mar 5, 2020

I see two solutions, where at least one is needed to fix the problem:

  • Restore the if ( d <= CD::zero() ) return cv_status::timeout;
  • Pass a valid timestruct to pthread_cond_timedwait (http://man7.org/linux/man-pages/man3/pthread_cond_timedwait.3p.html says the nsecs value can not be negative: "The abstime argument specified a nanosecond value less than zero or greater than or equal to 1000 million." )

@luke-jr
Copy link

luke-jr commented Mar 5, 2020

I'm not sure the clock is guaranteed to be positive, so I think the latter fix is probably correct?

luke-jr added a commit to bitcoinknots/bitcoin that referenced this issue Mar 5, 2020
Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout
See boostorg/thread#308
luke-jr added a commit to luke-jr/bitcoin that referenced this issue Mar 5, 2020
Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout
See boostorg/thread#308
luke-jr added a commit to luke-jr/bitcoin that referenced this issue Mar 6, 2020
Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout
See boostorg/thread#308
maflcko pushed a commit to bitcoin/bitcoin that referenced this issue Aug 28, 2020
…t's wait_until

ed0223e scheduler: Workaround negative nsecs bug in boost's wait_until (Luke Dashjr)

Pull request description:

  Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout
  See boostorg/thread#308

  NOTE: This was addressed in master with a refactor (#18234), so this isn't a strict backport and needs full review.

  Fixes #18227

  Cleanly merges to 0.14+

ACKs for top commit:
  laanwj:
    ACK ed0223e
  gruve-p:
    ACK ed0223e

Tree-SHA512: 57edd0a22d7cf8f04b427e23d1ba10746a492638021d4438781b9d313dd0459418f64f0489be72d8e2286bbc8e8762d77e673868c25eb3bf4f0423a8fe8cdffa
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this issue Dec 1, 2020
Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>

Summary
---

This fixes issue Bitcoin-ABC#180 and also re-enables the mockforward test disabled
by !837.

It is a backport from core with a workaround to boost bug:
boostorg/thread#308, which was the source of
the mockforward tests failing in Bitcoin-ABC#180.

Original core PR: bitcoin/bitcoin#18284

Original Core Commit Message
---

    Some boost versions have a bug that can cause a time prior to system boot
    (or wake from sleep) to throw an exception instead of return timeout
    See boostorg/thread#308

    NOTE: This was addressed in master with a refactor (#18234), so this isn't
    a strict backport and needs full review.

    Fixes #18227

    Cleanly merges to 0.14+

Test Plan
---

- `ninja check`

Advanced:

- If you can, try and get the mockforward test to fail without this
  fix, and then try and get it to fail again with this fix.  If it
  succeeds always with this fix we are golden.  The test is in
  "scheduled_tests".
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

No branches or pull requests

4 participants
@fanquake @luke-jr @maflcko and others