tests: Write the notification message to different files to avoid race condition in feature_notifications.py #14275

pull ken2812221 wants to merge 1 commits into bitcoin:master from ken2812221:2018-09-20-test-notification-sleep changing 2 files +27 −29
  1. ken2812221 commented at 8:27 am on September 20, 2018: contributor
    This PR change the behavior that feature_notifications.py would write to different files instead of writing to the same file to avoid race condition.
  2. fanquake added the label Tests on Sep 20, 2018
  3. DrahtBot commented at 9:18 am on September 20, 2018: member
    • #14320 (wallet: Fix duplicate fileid detection by ken2812221)
    • #14316 (tests: exclude all tests with difference parameters in --exclude list by ken2812221)

    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.

  4. MarcoFalke commented at 11:58 am on September 20, 2018: member
    utACK f8b6424a1014e8080f22b39a6d3b9998e85f2f90
  5. sdaftuar commented at 12:42 pm on September 20, 2018: member
    Could you explain the race condition?
  6. ken2812221 commented at 12:51 pm on September 20, 2018: contributor

    Could you explain the race condition?

    Bitcoin Core runs the each notification command in new thread, so it would have 10 process writing the same file if it generate 10 blocks immediately in this tests.

  7. MarcoFalke commented at 5:53 pm on September 20, 2018: member
    Maybe on a second thought the tests should mirror how the notifications are supposed to be used in practice. If we don’t support support filesystems that deny access when a lock is taken, as opposed to waiting, then the test shouldn’t run on that system.
  8. ryanofsky commented at 6:38 pm on September 20, 2018: member
    Instead of sleeping between the blocknotify commands, so they can all write to the same file, what about just having them write to different files. E.g. create a blocknotify/ directory and change -blocknotify=echo %s >> blocks.txt to -blocknotify=echo > blocknotify/%s
  9. promag commented at 10:34 pm on September 20, 2018: member

    @ryanofsky suggestion is good to fix the problem.

    But I wonder if we should replace the “call system with user command in a new thread” with a FIFO notification thread?

  10. ken2812221 force-pushed on Sep 21, 2018
  11. ken2812221 force-pushed on Sep 21, 2018
  12. ken2812221 force-pushed on Sep 21, 2018
  13. ken2812221 force-pushed on Sep 21, 2018
  14. ken2812221 renamed this:
    tests: Slowly generate blocks to avoid race condition in feature_notifications.py
    tests: Write the notification message to different files to avoid race condition in feature_notifications.py
    on Sep 21, 2018
  15. ken2812221 commented at 6:36 am on September 21, 2018: contributor
    @ryanofsky Updated the commit. Now the test create empty files with different name.
  16. in test/functional/feature_notifications.py:41 in 551b39a347 outdated
    38@@ -36,54 +39,49 @@ def run_test(self):
    39         blocks = self.nodes[1].generate(block_count)
    40 
    41         # wait at most 10 seconds for expected file size before reading the content
    


    sdaftuar commented at 7:08 pm on September 24, 2018:
    nit: comment should be updated: ...expected directory size...

    sdaftuar commented at 7:09 pm on September 24, 2018:
    also the comments below this one that refer to “file contents” should be “directory contents”
  17. sdaftuar commented at 7:50 pm on September 24, 2018: member
    The last commit looks good to me, modulo the comment nits. I have not reviewed the first two commits which are from #14007.
  18. DrahtBot added the label Needs rebase on Sep 24, 2018
  19. tests: write the notification to different files to avoid race condition 67654b6405
  20. ken2812221 force-pushed on Sep 24, 2018
  21. DrahtBot removed the label Needs rebase on Sep 24, 2018
  22. MarcoFalke commented at 5:24 pm on September 25, 2018: member
    utACK 67654b6405
  23. MarcoFalke merged this on Sep 25, 2018
  24. MarcoFalke closed this on Sep 25, 2018

  25. MarcoFalke referenced this in commit bb8c9b3f74 on Sep 25, 2018
  26. ken2812221 deleted the branch on Sep 25, 2018
  27. deadalnix referenced this in commit 0011ed83a2 on May 6, 2020
  28. ftrader referenced this in commit 5ac574978d on Aug 17, 2020
  29. 5tefan referenced this in commit ded574b89a on Aug 13, 2021
  30. 5tefan referenced this in commit ac3640899c on Aug 14, 2021
  31. PastaPastaPasta referenced this in commit f370f41210 on Aug 16, 2021
  32. MarcoFalke locked this on Sep 8, 2021

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-19 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me