Always enforce strict lock ordering (try or not) #9674

pull TheBlueMatt wants to merge 4 commits into bitcoin:master from TheBlueMatt:2017-02-inv-send-lockorder changing 2 files +20 −48
  1. TheBlueMatt commented at 1:04 AM on February 3, 2017: member

    We're pretty close to using consistent lock ordering even within try locks, it seems. This is a simple fixup to match the lockorders used elsewhere.

  2. Lock cs_vSend and cs_inventory in a consistent order even in TRY fd13eca147
  3. pstratem commented at 1:13 AM on February 3, 2017: contributor

    Does it even matter if TRYLOCK ordering is consistent?

  4. dcousens commented at 1:18 AM on February 3, 2017: contributor

    @pstratem if the ordering is inconsistent, you could have a situation where two threads have both locked their respective first lock, and unable to gain the second.

  5. pstratem commented at 1:23 AM on February 3, 2017: contributor

    @dcousens yeah but they're try locks, so they'll fail and try again and one will eventually succeed

  6. dcousens commented at 1:29 AM on February 3, 2017: contributor

    @pstratem in that situation, wouldn't they both succeed on the first, and continually try again on the second [if the ordering was inconsistent]?

    I suppose you're right, it would get through eventually, but, it does seem better to avoid that if possible no?

  7. sipa commented at 1:39 AM on February 3, 2017: member

    A thread doing (L1, try L2) and another doing (L2, L1) is not a problem. A thread doing (try L1, L2) and another doing (L2, L1) is a problem.

  8. MarcoFalke added the label Refactoring on Feb 3, 2017
  9. Always enforce lock strict lock ordering (try or not) 8465631845
  10. TheBlueMatt commented at 3:33 AM on February 3, 2017: member

    No, in this case it does not matter, but for code hygene we should generally try to (and it can matter, as @sipa points out).

    Turns out that, at least for our travis test cases, this lock inversion was the only one left, so DEBUG_LOCKORDER now enforces for all lock inversions.

  11. TheBlueMatt renamed this:
    Lock cs_vSend and cs_inventory in a consistent order even in TRY
    Always enforce strict lock ordering (try or not)
    on Feb 3, 2017
  12. laanwj commented at 8:09 AM on February 3, 2017: member

    Well it simplifies the checking code at least that's a good thing...

  13. TheBlueMatt commented at 12:44 PM on February 3, 2017: member

    The checking code is now also able to detect some deadlocks that it previously was unable to, though I'd have to go stare at it for a while to recall which those were.

    On February 3, 2017 3:09:43 AM EST, "Wladimir J. van der Laan" notifications@github.com wrote:

    Well it simplifies the checking code at least that's a good thing...

    -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9674#issuecomment-277188226

  14. in src/net.cpp:None in 8465631845 outdated 
    1069 | @@ -1070,12 +1070,13 @@ void CConnman::ThreadSocketHandler()
1070 |                  {
1071 |                      bool fDelete = false;
1072 |                      {
1073 | -                        TRY_LOCK(pnode->cs_vSend, lockSend);
1074 | -                        if (lockSend)
1075 | +                        TRY_LOCK(pnode->cs_inventory, lockInv);
1076 | +                        if (lockInv)
    sipa commented at 12:47 AM on February 4, 2017:

    Can you fix style here?

  15. sipa commented at 12:51 AM on February 4, 2017: member

    Nice, utACK 8465631845eac3db834942a4feb50f65c3401c68.

  16. Fixup style a bit by moving { to the same line as if statements 2a962d4540
  17. TheBlueMatt commented at 9:48 PM on February 4, 2017: member

    Pushed a commit that purely does style fixes (moves { up to the same line as if). Would like to see the agressive DEBUG_LOCKORDER assertions in 0.14rc1 for testers, but its not the end of the world to miss.

  18. gmaxwell commented at 10:37 PM on February 4, 2017: contributor

    I think this should go in 0.14. utACK.

  19. dcousens approved
  20. Further-enforce lockordering by enforcing directly after TRY_LOCKs 618ee9249b
  21. in src/sync.cpp:None in 8465631845 outdated 
      81 | -    // a try lock will immediately have otherwise bailed if it had
  82 | -    // failed to get the lock
  83 | -    // We do this by, for the locks which triggered the potential deadlock,
  84 | -    // in either lockorder, checking that the second of the two which is locked
  85 | -    // is only a TRY_LOCK, ignoring locks if they are reentrant.
  86 | -    bool firstLocked = false;
    ryanofsky commented at 6:58 PM on February 7, 2017:

    Do you need to get rid of the if (!fTry) condition below in push_lock() to make this happen with trylocks?

    Also, it doesn't appear to that invlockorders variable is ever used here. Can it be deleted?

    ryanofsky commented at 7:09 PM on February 7, 2017:

    Never mind about invlockorders, seems to be needed to clean up lockorders when the lock is deleted.

  22. gmaxwell commented at 4:11 AM on February 8, 2017: contributor

    ACK.

  23. theuni commented at 6:56 AM on February 8, 2017: member

    utACK 618ee9249b178d94911ea66cb4b5291f000ef1fb

  24. laanwj merged this on Feb 8, 2017
  25. laanwj closed this on Feb 8, 2017
  26. laanwj referenced this in commit dd163f5788 on Feb 8, 2017
  27. laanwj commented at 1:49 PM on February 8, 2017: member

    Would like to see the agressive DEBUG_LOCKORDER assertions in 0.14rc1 for testers

    Note that we don't build releases (and neither release candidates) with DEBUG_LOCKORDER enabled.

  28. TheBlueMatt commented at 2:03 PM on February 8, 2017: member

    I'm aware, but I hope some testers do, if only other devs.

    On February 8, 2017 8:49:27 AM EST, "Wladimir J. van der Laan" notifications@github.com wrote:

    Would like to see the agressive DEBUG_LOCKORDER assertions in 0.14rc1 for testers

    Note that we don't build releases (and neither release candidates) with DEBUG_LOCKORDER enabled.

    -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9674#issuecomment-278333394

  29. laanwj referenced this in commit 058c0f996b on Oct 2, 2017
  30. codablock referenced this in commit 14aae6058e on Jan 19, 2018
  31. codablock referenced this in commit 7d4e34029c on Jan 23, 2018
  32. HashUnlimited referenced this in commit 95fe59dec7 on Feb 27, 2018
  33. andvgal referenced this in commit 845d3890c6 on Jan 6, 2019
  34. CryptoCentric referenced this in commit 4ac24c27f1 on Feb 27, 2019
  35. PastaPastaPasta referenced this in commit fa7c71ac02 on Dec 22, 2019
  36. PastaPastaPasta referenced this in commit aaae79b0f8 on Jan 2, 2020
  37. PastaPastaPasta referenced this in commit 35c7897cb8 on Jan 4, 2020
  38. PastaPastaPasta referenced this in commit 7a1bc16cab on Jan 12, 2020
  39. PastaPastaPasta referenced this in commit c6c35849c7 on Jan 12, 2020
  40. PastaPastaPasta referenced this in commit 0e7ceed105 on Jan 12, 2020
  41. PastaPastaPasta referenced this in commit de032e62ed on Jan 12, 2020
  42. PastaPastaPasta referenced this in commit f0b901d363 on Jan 12, 2020
  43. PastaPastaPasta referenced this in commit 46a1ad7939 on Jan 12, 2020
  44. furszy referenced this in commit 14f0e974f8 on Oct 17, 2020
  45. ckti referenced this in commit a6b28229f2 on Mar 28, 2021
  46. MarcoFalke locked this on Sep 8, 2021
Contributors
TheBlueMattpstratemdcousenssipalaanwjgmaxwellryanofskytheuni
Labels
Refactoring
Linked (view graph)
#11432 Remove unused fTry from push_lock
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: 2026-04-22 00:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me