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

Feature notifications timeout fix #712

Merged

Conversation

dsaveliev
Copy link
Member

@dsaveliev dsaveliev commented Mar 1, 2019

Fixes #373
This is a port of bitcoin/bitcoin#14670

A timeout occurs during blocks generation in "feature_notifications" test.
I suspect that it might happen due to the log file locking - "BlockNotifyCallback" is executing the notification command in a separate thread and each command tries to write data to the same log file asynchronously.
The aforementioned port should help to eliminate such kind of errors.

Test run:
https://travis-ci.com/dsaveliev/unit-e/jobs/181612551

Signed-off-by: Dmitry Saveliev dima@thirdhash.com

@dsaveliev dsaveliev added the bug A problem of existing functionality label Mar 1, 2019
@dsaveliev dsaveliev added this to the 0.1 milestone Mar 1, 2019
@dsaveliev dsaveliev marked this pull request as ready for review March 1, 2019 15:38
@dsaveliev dsaveliev force-pushed the feature-notifications-timeout-fix branch from a894b00 to 78707e5 Compare March 4, 2019 08:56
@Ruteri Ruteri requested a review from a team March 4, 2019 10:29
Copy link
Member

@cmihai cmihai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 78707e5 , regardless of my misgivings about -alertnotify and -blocknotify features themselves, which allow running arbitrary shell commands on the user's machine.

Note: Someone with admin access to the repository should merge this PR without squashing, to preserve commit authorship.

@dsaveliev
Copy link
Member Author

regardless of my misgivings about -alertnotify and -blocknotify features themselves, which allow running arbitrary shell commands on the user's machine.

I agree on that, but this is a separate, big topic for discussion.

Copy link
Member

@Ruteri Ruteri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note from the original PR bitcoin/bitcoin#14275 (comment)

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.

utACK 78707e5

@dsaveliev dsaveliev merged commit 5fdd8bc into dtr-org:master Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A problem of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants