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

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.
fanquake added the label Tests on Sep 20, 2018
DrahtBot commented at 9:18 AM on September 20, 2018: contributor
Note to reviewers: This pull request conflicts with the following ones:
- #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.
DrahtBot cross-referenced this on Sep 20, 2018 from issue appveyor: script improvement by ken2812221
DrahtBot cross-referenced this on Sep 20, 2018 from issue Fix for test runner UnicodeDecodeError when utf-8 is not supported by sanket1729
DrahtBot cross-referenced this on Sep 20, 2018 from issue tests: Run functional test on Windows and enable it on Appveyor by ken2812221
MarcoFalke commented at 11:58 AM on September 20, 2018: member

utACK f8b6424a1014e8080f22b39a6d3b9998e85f2f90
sdaftuar commented at 12:42 PM on September 20, 2018: member

Could you explain the race condition?
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.
sdaftuar cross-referenced this on Sep 20, 2018 from issue Callback/notification documentation and cleanup by sdaftuar
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.
ryanofsky commented at 6:38 PM on September 20, 2018: contributor

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
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?
ken2812221 force-pushed on Sep 21, 2018
ken2812221 force-pushed on Sep 21, 2018
ken2812221 force-pushed on Sep 21, 2018
ken2812221 force-pushed on Sep 21, 2018
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
ken2812221 commented at 6:36 AM on September 21, 2018: contributor

@ryanofsky Updated the commit. Now the test create empty files with different name.
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"
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.
DrahtBot added the label Needs rebase on Sep 24, 2018
tests: write the notification to different files to avoid race condition 67654b6405
ken2812221 force-pushed on Sep 24, 2018
DrahtBot removed the label Needs rebase on Sep 24, 2018
DrahtBot cross-referenced this on Sep 25, 2018 from issue tests: exclude all tests with difference parameters in `--exclude` list by ken2812221
DrahtBot cross-referenced this on Sep 25, 2018 from issue [bugfix] wallet: Fix duplicate fileid detection by ken2812221
MarcoFalke commented at 5:24 PM on September 25, 2018: member

utACK 67654b6405
MarcoFalke merged this on Sep 25, 2018
MarcoFalke closed this on Sep 25, 2018
MarcoFalke referenced this in commit bb8c9b3f74 on Sep 25, 2018
ken2812221 deleted the branch on Sep 25, 2018
Bushstar cross-referenced this on Oct 11, 2018 from issue Updates from bitcoin/master by Bushstar
dsaveliev cross-referenced this on Mar 1, 2019 from issue Feature notifications timeout fix by dsaveliev
dsaveliev cross-referenced this on Mar 1, 2019 from issue tests: Allow closed http server in assert_start_raises_init_error by dsaveliev
deadalnix referenced this in commit 0011ed83a2 on May 6, 2020
ftrader referenced this in commit 5ac574978d on Aug 17, 2020
5tefan referenced this in commit ded574b89a on Aug 13, 2021
5tefan referenced this in commit ac3640899c on Aug 14, 2021
PastaPastaPasta referenced this in commit f370f41210 on Aug 16, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#14007 tests: Run functional test on Windows and enable it on Appveyor #14278 Callback/notification documentation and cleanup