Relog configuration args on debug.log rotation #16673
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
fe88d65 to
410a617
Compare
b217bf9 to
d252dfe
Compare
| @@ -307,7 +307,7 @@ def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT): | |||
| wait_until(self.is_node_stopped, timeout=timeout) | |||
|
|
|||
| @contextlib.contextmanager | |||
| def assert_debug_log(self, expected_msgs, timeout=2): | |||
| def assert_debug_log(self, expected_msgs, unexpected_msgs=[], timeout=2): | |||
There was a problem hiding this comment.
Do you really want unexpected_msgs to preserve parameter state between calls? :-)
>>> def foobar(foo, bar=[]):
... bar.append(foo)
... print(bar)
...
>>> foobar("hello")
['hello']
>>> foobar("hello")
['hello', 'hello']
>>> foobar("hello")
['hello', 'hello', 'hello']
I think this is what you want:
def assert_debug_log(self, expected_msgs, unexpected_msgs, timeout=2):
if unexpected_msgs is None:
unexpected_msgs = []
FWIW I think we should use a linter to guard against this Python gotcha :-)
There was a problem hiding this comment.
Great catch, I had no idea! I fixed this in #16115 (where it was introduced) and force-pushed rebased this PR onto that branch.
There was a problem hiding this comment.
@practicalswift, I don't think this was an actual problem. I added a debug print and unexpected_msgs didn't include previous values. I think it's not broken because this function, assert_debug_log(), doesn't modify the variable (it's not mutable). Here's an extremely simplified version of this function and some sample calls to it:
def assert_debug_log(unexpected_msgs=[]):
print(unexpected_msgs)
for unexpected_msg in unexpected_msgs:
print(' ', unexpected_msg)
assert_debug_log()
assert_debug_log(["foo"])
assert_debug_log(unexpected_msgs=["foo"])
assert_debug_log("bar") # not actually called this way but just curious
assert_debug_log()
The output: 10000
[]
['foo']
foo
['foo']
foo
bar
b
a
r
[]
What prompted me to investigate this further is that I wondered if any of our existing python code has this problem. I found just one instance, which is in wallet_importmulti.py:
def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
But adding a debug print to that function showed everything was okay. I believe that's because warnings isn't modified in that function.
d252dfe to
5b413fd
Compare
maflcko
pushed a commit
that referenced
this pull request
Aug 28, 2019
…n mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * #16673 (comment) * #14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 28, 2019
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
5b413fd to
34c97d7
Compare
34c97d7 to
c0fa4be
Compare
c0fa4be to
339a1e3
Compare
339a1e3 to
6a008dd
Compare
6a008dd to
1375f89
Compare
1375f89 to
3fe5ba8
Compare
3fe5ba8 to
773660c
Compare
LarryRuane
commented
Jan 19, 2020
773660c to
fc7c03c
Compare
f0ce638 to
20dbb95
Compare
|
Force-pushed required rebase. I should have mentioned earlier that you can test this manually by starting Then check A more realistic test is is to do actual log rotation: Start (Note that in this state, additional logging will go to Then send the signal. A new |
Closed
|
NACK - does not seem useful or necessary (given the previous logs can be referred to easily). |
Well, if it's not useful or necessary, I don't think it's for this reason, because an external log rotation agent (see |
ah, I see.. yes, in that case this might be useful, but I think it's better if it's optional - or you could just use a logrotate script to save this part of the debug.log file elsewhere before it deletes the file. |
This was referenced Mar 2, 2022
20dbb95 to
4eededb
Compare
|
Force-pushed rebase, and made a few improvements. |
4eededb to
a4e6c45
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened. |
knst
pushed a commit
to knst/dash
that referenced
this pull request
Feb 13, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Feb 13, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Feb 28, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Feb 28, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 14, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 15, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
PastaPastaPasta
pushed a commit
to knst/dash
that referenced
this pull request
Apr 4, 2023
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was part of #16115 but reviewers requested that this be made into a separate PR. As explained in #16115, it's useful, maybe even crucial, for support, analysis, and debugging purposes for the configuration arguments to be present in the
debug.logfile, as that may be the only artifact provided to the developer or support team. That PR logs the configuration settings (command-line arguments andbitcoin.confcontents) whenbitcoindstarts. But sometime later, the debug log file may be "rotated" (a newdebug.logfile started so it doesn't grow unbounded in size) and this information would be lost unless the previousdebug.logis preserved.This PR re-logs the configuration information when the log rotates, ensuring that the current
debug.logfile contains this information. Reviewers had reservations about the previous approach (which you can see in #16115's discussion thread), so this PR implements this functionality in a simpler and (I believe) much better way.