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

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

Merged

Conversation

Projects
None yet
7 participants
@ken2812221
Copy link
Member

commented Sep 20, 2018

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 fanquake added the Tests label Sep 20, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

utACK f8b6424a1014e8080f22b39a6d3b9998e85f2f90

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Could you explain the race condition?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

@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 ken2812221 force-pushed the ken2812221:2018-09-20-test-notification-sleep branch 4 times, most recently Sep 21, 2018

@ken2812221 ken2812221 changed the title 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 Sep 21, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2018

@ryanofsky Updated the commit. Now the test create empty files with different name.

@sdaftuar
Copy link
Member

left a comment

The last commit looks good to me, modulo the comment nits. I have not reviewed the first two commits which are from #14007.

test/functional/feature_notifications.py Outdated
@@ -36,54 +39,49 @@ def run_test(self):
blocks = self.nodes[1].generate(block_count)

# wait at most 10 seconds for expected file size before reading the content

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Sep 24, 2018

Member

nit: comment should be updated: ...expected directory size...

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Sep 24, 2018

Member

also the comments below this one that refer to "file contents" should be "directory contents"

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

utACK 67654b6

@MarcoFalke MarcoFalke merged commit 67654b6 into bitcoin:master Sep 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 25, 2018

Merge #14275: tests: Write the notification message to different file…
…s to avoid race condition in feature_notifications.py

67654b6 tests: write the notification to different files to avoid race condition (Chun Kuan Lee)

Pull request description:

  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.

Tree-SHA512: 78406167cc6a3f570134b0ee76d2be1440bc1498cd7b1be72fae16d0ab86950e26ef3bf6008796016e5418231400c6492f0e062909dd882646541ecb7a70fb30

@ken2812221 ken2812221 deleted the ken2812221:2018-09-20-test-notification-sleep branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.