util: Replace boost sleep with std sleep #16117

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:1905-noBoostUtilTime changing 16 files +22 −105
  1. MarcoFalke commented at 12:23 pm on May 29, 2019: member
    We don’t use the interruptible feature of boost’s sleep anywhere, so replace it with the sleep in std::thread
  2. MarcoFalke added the label Refactoring on May 29, 2019
  3. MarcoFalke added the label Utils/log/libs on May 29, 2019
  4. in src/util/time.cpp:83 in fa12affe91 outdated 
    87-/**
88- * Boost's sleep_for was uninterruptible when backed by nanosleep from 1.50
89- * until fixed in 1.52. Use the deprecated sleep method for the broken case.
90- * See: https://svn.boost.org/trac/boost/ticket/7238
91- */
92-#if defined(HAVE_WORKING_BOOST_SLEEP_FOR)
    laanwj commented at 12:40 pm on May 29, 2019:
    oh wow, can we finally get rid of this abomination? feel free to delete the detection of this as well from the build system!
    MarcoFalke commented at 1:07 pm on May 29, 2019:
    Good idea! Done.
  5. MarcoFalke force-pushed on May 29, 2019
  6. MarcoFalke force-pushed on May 29, 2019
  7. MarcoFalke commented at 1:08 pm on May 29, 2019: member
    Removed unused configure checks
  8. fanquake requested review from theuni on May 29, 2019
  9. sipa commented at 4:14 pm on May 29, 2019: member

    Are the std:: versions compatible with our use of boost threadgroup interrupts?

    EDIT: ping @theuni

  10. DrahtBot commented at 5:13 pm on May 29, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18264 ([WIP] build: Remove Boost Chrono by fanquake)
    • #18234 (refactor: Replace boost::mutex,condition_var,chrono with std equivalents in scheduler by ajtowns)
    • #18149 (build: add –enable-isystem and change –enable-werror to enable -Werror by vasild)
    • #18011 (Replace current benchmarking framework with nanobench by martinus)
    • #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
    • #14920 (Build: enable -Wdocumentation via isystem by Empact)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  11. laanwj commented at 7:27 am on May 30, 2019: member

    Are the std:: versions compatible with our use of boost threadgroup interrupts?

    Oh crap I knew there was a reason we hadn’t got rid of the boost::sleep madness yet :cry:

    I don’t think the std:: sleep allows any way to interrupt it, at all?

    I think the non-boost approach would be to have them timed-wait on a condition variable.

  12. practicalswift commented at 1:29 pm on May 30, 2019: contributor

    Hopefully that has no observable effects other than improved code clarity, faster compilation and faster run time.

    There is a very observable effect of this PR on 32-bit systems:

    Prior to this PR bitcoind would shut down with an assertion failure when the 32-bit time_t overflow occurs.

    After this PR bitcoind will keep running but with negative time returned from these functions :-)

  13. practicalswift commented at 1:41 pm on May 30, 2019: contributor

    Perhaps we should add a test making sure bitcoind on 32-bit systems behave the way we want it to – which I assume is immediate shutdown? – at time of the 32-bit time_t overflow event.

    Related: RFC: Improving testing under the remaining supported 32 bit platforms

  14. laanwj commented at 3:18 pm on May 30, 2019: member

    How do other programs handle this? I though Linux switched to 64 bit timer values on 32-bit systems internally a while ago (and Linux is the only 32 bit platform supported), but I see on ARM time_t is still 32 bit.

    I mean it must be possible to handle time correctly on 32-bit systems post-2038—right?

    Edit: no, apparently this is not possible?!? Threads from 2015 made my hopeful: https://lwn.net/Articles/643234/ but I don’t think this was actually addressed yet, I think.

  15. MarcoFalke commented at 11:15 pm on May 30, 2019: member

    After this PR bitcoind will keep running but with negative time returned from these functions :-)

    Can you please explain a bit more why this happens. Be reminded that the predefined std::chrono::durations use an underlying integer type that is large enough (for year 2038 to be of no problem).

  16. practicalswift commented at 7:23 am on June 6, 2019: contributor

    After this PR bitcoind will keep running but with negative time returned from these functions :-)

    Can you please explain a bit more why this happens. Be reminded that the predefined std::chrono::durations use an underlying integer type that is large enough (for year 2038 to be of no problem).

    As long as the syscall used returns 32-bit time (since epoch) on 32-bit systems this cannot be solved no matter what integer types are used to abstract time in the standard library, right? :-)

  17. MarcoFalke closed this on Jun 10, 2019
  18. MarcoFalke deleted the branch on Jun 10, 2019
  19. MarcoFalke commented at 7:19 am on June 10, 2019: member
    Yeah, and the you can’t run the underlying system anyway. There is no need to try to figure out how to make Bitcoin Core work on a broken system.
  20. practicalswift commented at 12:10 pm on June 10, 2019: contributor
    @MarcoFalke Perhaps we should start thinking about a deprecation plan for 32-bit systems (see #16096)? Even if this is 19 years away software tends to live longer than originally anticipated :-)
  21. MarcoFalke restored the branch on Feb 21, 2020
  22. MarcoFalke reopened this on Feb 21, 2020
  23. util: Add UnintrruptibleSleep fa4620be78
  24. scripted-diff: Replace MilliSleep with UninterruptibleSleep 
    This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.

* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
    fa9af06d91
  25. util: Remove unused MilliSleep fae86c38bc
  26. MarcoFalke force-pushed on Feb 21, 2020
  27. MarcoFalke force-pushed on Feb 21, 2020
  28. MarcoFalke renamed this:
    util: Replace boost:: with std:: in utiltime
    util: Replace boost sleep with std sleep
    on Feb 21, 2020
  29. MarcoFalke commented at 6:42 pm on February 21, 2020: member
    I’ve removed the changes on the time getters, so that this doesn’t drift off into a discussion about the y2038 problem
  30. MarcoFalke added this to the "In progress" column in a project
  31. practicalswift commented at 9:01 am on February 23, 2020: contributor
    ACK fae86c38bca5c960462e53975314a0749db5d17d – patch looks correct
  32. sipa commented at 6:12 am on March 4, 2020: member
    Concept and code review ACK fae86c38bca5c960462e53975314a0749db5d17d
  33. fanquake approved
  34. fanquake commented at 6:54 am on March 6, 2020: member
    ACK fae86c38bca5c960462e53975314a0749db5d17d - note that an instance of DHAVE_WORKING_BOOST_SLEEP_FOR was missed in the linter, but that can be cleaned up later.
  35. ajtowns commented at 7:15 am on March 6, 2020: member
    ACK fae86c38bca5c960462e53975314a0749db5d17d quick code review
  36. fanquake merged this on Mar 6, 2020
  37. fanquake closed this on Mar 6, 2020
  38. MarcoFalke deleted the branch on Mar 6, 2020
  39. laanwj commented at 6:50 pm on March 6, 2020: member
    Postmortem ACK.
  40. MarcoFalke moved this from the "In progress" to the "Done" column in a project
  41. sidhujag referenced this in commit 42c06c92fd on Mar 7, 2020
  42. deadalnix referenced this in commit 4b5f81f800 on Jun 11, 2020
  43. deadalnix referenced this in commit 5a8dfec98f on Jun 11, 2020
  44. deadalnix referenced this in commit 11bc97a47f on Jun 11, 2020
  45. ftrader referenced this in commit a8748fce7c on Aug 17, 2020
  46. sidhujag referenced this in commit 7ae631b27c on Nov 10, 2020
  47. kittywhiskers referenced this in commit 9cfae9d744 on Jul 2, 2021
  48. kittywhiskers referenced this in commit 0019ec63b9 on Jul 4, 2021
  49. kittywhiskers referenced this in commit 28fb1a387b on Jul 9, 2021
  50. kittywhiskers referenced this in commit 49925e52a5 on Jul 9, 2021
  51. kittywhiskers referenced this in commit b46b64141c on Jul 9, 2021
  52. kittywhiskers referenced this in commit f0ce3afb73 on Jul 13, 2021
  53. kittywhiskers referenced this in commit 1826c3b554 on Jul 15, 2021
  54. kittywhiskers referenced this in commit b6a2e782a4 on Jul 16, 2021
  55. kittywhiskers referenced this in commit 1ad6c5e9c7 on Aug 1, 2021
  56. kittywhiskers referenced this in commit b36b489ad0 on Aug 5, 2021
  57. kittywhiskers referenced this in commit cbc4a74250 on Aug 5, 2021
  58. UdjinM6 referenced this in commit 7aebf156e9 on Aug 10, 2021
  59. 5tefan referenced this in commit 4204a6553f on Aug 12, 2021
  60. furszy referenced this in commit c8ad2c17ab on Aug 27, 2021
  61. DrahtBot locked this on Feb 15, 2022


MarcoFalke laanwj sipa DrahtBot practicalswift fanquake ajtowns


theuni

Labels
Refactoring Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me