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: Stop node before removing the notification file #14465

Merged
merged 1 commit into from Oct 19, 2018

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Oct 11, 2018

Stop node before removing the notification file to make sure the command has been terminated. After then we could removing those files safely and do not receive any permission error. (See #14446)

The permission error is Windows specific, documented in python doc:

On Windows, attempting to remove a file that is in use causes an exception to be raised

See https://docs.python.org/3/library/os.html#os.remove

@promag
Copy link
Member

@promag promag commented Oct 11, 2018

ACK. Have you checked for other cases?

@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Oct 11, 2018

Have you checked for other cases?

Yes, but still in investigation.

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Oct 11, 2018

LGTM, utACK ca6d86c

@sipa
Copy link
Member

@sipa sipa commented Oct 12, 2018

utACK

Copy link
Contributor

@conscott conscott left a comment

Possible other case in the mempool persist test ? Although in this case, removing an active file is intentional.

@fanquake fanquake requested a review from MarcoFalke Oct 18, 2018
@sipa sipa merged commit ca6d86c into bitcoin:master Oct 19, 2018
2 checks passed
sipa added a commit that referenced this issue Oct 19, 2018
ca6d86c tests: Stop node before removing the notification file (Chun Kuan Lee)

Pull request description:

  Stop node before removing the notification file to make sure the command has been terminated. After then we could removing those files safely and do not receive any permission error. (See #14446)

  The permission error is Windows specific, documented in python doc:
  >On Windows, attempting to remove a file that is in use causes an exception to be raised

  See https://docs.python.org/3/library/os.html#os.remove

Tree-SHA512: fbdabf3a9a838bb59ba207dd9e9fbdd87c702a99ad66bee0b2b1537f80f8630d22d9d5e9c4ded23a82a66bfc10989227fb024b27393425abe0e5a2ad4e4cbb82
@ken2812221 ken2812221 deleted the test-notification-fix branch Oct 19, 2018
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jul 30, 2020
Summary:
Stop node before removing the notification file (Chun Kuan Lee)

Pull request description:

  Stop node before removing the notification file to make sure the command has been terminated. After then we could removing those files safely and do not receive any permission error. (See #14446)

  The permission error is Windows specific, documented in python doc:
  >On Windows, attempting to remove a file that is in use causes an exception to be raised

  See https://docs.python.org/3/library/os.html#os.remove

bitcoin/bitcoin@ca6d86c

---

Backport of Core [[bitcoin/bitcoin#14465 | PR14465]]

Test Plan:
  ninja
  test_runner.py feature_notifications

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7077
5tefan added a commit to 5tefan/dash that referenced this issue Aug 13, 2021
…n file

ca6d86c tests: Stop node before removing the notification file (Chun Kuan Lee)

Pull request description:

  Stop node before removing the notification file to make sure the
command has been terminated. After then we could removing those files
safely and do not receive any permission error. (See bitcoin#14446)

  The permission error is Windows specific, documented in python doc:
>On Windows, attempting to remove a file that is in use causes an
exception to be raised

  See https://docs.python.org/3/library/os.html#os.remove

Tree-SHA512: fbdabf3a9a838bb59ba207dd9e9fbdd87c702a99ad66bee0b2b1537f80f8630d22d9d5e9c4ded23a82a66bfc10989227fb024b27393425abe0e5a2ad4e4cbb82
5tefan added a commit to 5tefan/dash that referenced this issue Aug 14, 2021
…n file

ca6d86c tests: Stop node before removing the notification file (Chun Kuan Lee)

Pull request description:

  Stop node before removing the notification file to make sure the
command has been terminated. After then we could removing those files
safely and do not receive any permission error. (See bitcoin#14446)

  The permission error is Windows specific, documented in python doc:
>On Windows, attempting to remove a file that is in use causes an
exception to be raised

  See https://docs.python.org/3/library/os.html#os.remove

Tree-SHA512: fbdabf3a9a838bb59ba207dd9e9fbdd87c702a99ad66bee0b2b1537f80f8630d22d9d5e9c4ded23a82a66bfc10989227fb024b27393425abe0e5a2ad4e4cbb82
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants