Skip to content

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
Contributor

promag commented Oct 11, 2018

ACK. Have you checked for other cases?

@ken2812221
Copy link
Contributor Author

Have you checked for other cases?

Yes, but still in investigation.

@meshcollider
Copy link
Contributor

LGTM, utACK ca6d86c

@sipa
Copy link
Member

sipa commented Oct 12, 2018

utACK

Copy link
Contributor

@conscott conscott left a comment

Choose a reason for hiding this comment

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

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

@fanquake fanquake requested a review from maflcko October 18, 2018 02:01
@sipa sipa merged commit ca6d86c into bitcoin:master Oct 19, 2018
sipa added a commit that referenced this pull request 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 October 19, 2018 05:59
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request 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 pull request 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

Development

Successfully merging this pull request may close these issues.

6 participants