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

ken2812221 · 2018-09-20T08:27:08Z

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.

DrahtBot · 2018-09-20T09:18:00Z

Note to reviewers: This pull request conflicts with the following ones:

[bugfix] wallet: Fix duplicate fileid detection #14320 (wallet: Fix duplicate fileid detection by ken2812221)
tests: exclude all tests with difference parameters in --exclude list #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.

maflcko · 2018-09-20T11:58:00Z

utACK f8b6424a1014e8080f22b39a6d3b9998e85f2f90

sdaftuar · 2018-09-20T12:42:04Z

Could you explain the race condition?

ken2812221 · 2018-09-20T12:51:22Z

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.

maflcko · 2018-09-20T17:53:52Z

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 · 2018-09-20T18:38:25Z

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 · 2018-09-20T22:34:53Z

@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 · 2018-09-21T06:36:48Z

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

sdaftuar

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

sdaftuar · 2018-09-24T19:08:28Z

test/functional/feature_notifications.py

@@ -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


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

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

maflcko · 2018-09-25T17:24:28Z

utACK 67654b6

…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

Summary: Backport of core [[bitcoin/bitcoin#14275 | PR14275]]. This is slighly adapted to not reduce large fork coverage but allow the test to pass on windows. Test Plan: ./test/functional/test_runner.py feature_notifications Ran on Linux, windows (depends on D5964) and OSX for sanity. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5966

…nt files 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

Merge bitcoin#14291, bitcoin#14275, bitcoin#14465, bitcoin#14559, bitcoin#14561

fanquake added the Tests label Sep 20, 2018

This was referenced Sep 20, 2018

appveyor: script improvement #14241

Merged

Fix for test runner UnicodeDecodeError when utf-8 is not supported #14237

Closed

tests: Run functional test on Windows and enable it on Appveyor #14007

Merged

sdaftuar mentioned this pull request Sep 20, 2018

Callback/notification documentation and cleanup #14278

Open

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

sdaftuar reviewed Sep 24, 2018

View reviewed changes

DrahtBot added the Needs rebase label Sep 24, 2018

tests: write the notification to different files to avoid race condition

67654b6

DrahtBot removed the Needs rebase label Sep 24, 2018

This was referenced Sep 25, 2018

tests: exclude all tests with difference parameters in --exclude list #14316

Merged

[bugfix] wallet: Fix duplicate fileid detection #14320

Merged

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

ken2812221 deleted the 2018-09-20-test-notification-sleep branch September 25, 2018 17:26

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

This was referenced Mar 1, 2019

Feature notifications timeout fix dtr-org/unit-e#712

Merged

tests: Allow closed http server in assert_start_raises_init_error dtr-org/unit-e#713

Merged

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Aug 16, 2021

Merge pull request #4339 from 5tefan/bp018a

f370f41

Merge bitcoin#14291, bitcoin#14275, bitcoin#14465, bitcoin#14559, bitcoin#14561

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ken2812221 commented Sep 20, 2018 •

edited

Loading

Uh oh!

DrahtBot commented Sep 20, 2018 •

edited

Loading

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

sdaftuar commented Sep 20, 2018

Uh oh!

ken2812221 commented Sep 20, 2018 •

edited

Loading

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

ryanofsky commented Sep 20, 2018

Uh oh!

promag commented Sep 20, 2018 •

edited

Loading

Uh oh!

ken2812221 commented Sep 21, 2018

Uh oh!

sdaftuar left a comment

Uh oh!

sdaftuar Sep 24, 2018

Uh oh!

sdaftuar Sep 24, 2018

Uh oh!

maflcko commented Sep 25, 2018

Uh oh!

Uh oh!

		@@ -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

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

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

Uh oh!

Conversation

ken2812221 commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

sdaftuar commented Sep 20, 2018

Uh oh!

ken2812221 commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

ryanofsky commented Sep 20, 2018

Uh oh!

promag commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Sep 21, 2018

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

sdaftuar Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 25, 2018

Uh oh!

Uh oh!

ken2812221 commented Sep 20, 2018 •

edited

Loading

DrahtBot commented Sep 20, 2018 •

edited

Loading

ken2812221 commented Sep 20, 2018 •

edited

Loading

promag commented Sep 20, 2018 •

edited

Loading