Skip to content

shutdown: Destroy kernel last, make test shutdown order consistent#28224

Merged
achow101 merged 2 commits intobitcoin:masterfrom
sedited:testSetupDestructorOrder
Nov 7, 2023
Merged

shutdown: Destroy kernel last, make test shutdown order consistent#28224
achow101 merged 2 commits intobitcoin:masterfrom
sedited:testSetupDestructorOrder

Conversation

@sedited
Copy link
Contributor

@sedited sedited commented Aug 5, 2023 •
edited by fanquake
Loading

The destruction/resetting of node context members in the tests should roughly follow the behavior of the Shutdown function in init.cpp.

This was originally requested by MarcoFalke in this comment in response to the original pull request introducing the kernel::Context.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 5, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ryanofsky, maflcko, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Aug 5, 2023
ryanofsky
ryanofsky approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 960112d

@maflcko
Copy link
Member

maflcko commented Oct 20, 2023

Are you still working on this?

@sedited
Copy link
Contributor Author

sedited commented Oct 20, 2023

Yes.

@maflcko
Copy link
Member

maflcko commented Oct 20, 2023

I think there is an outstanding comment: #28224 (comment) ?

@sedited sedited force-pushed the testSetupDestructorOrder branch from 960112d to 99a668c Compare October 21, 2023 10:40
@sedited
Copy link
Contributor Author

sedited commented Oct 21, 2023

Updated 960112d -> 99a668c (testSetupDestructorOrder_0 -> testSetupDestructorOrder_1, compare)

  • Addressed @ryanofsky's comment, ammending commit to adjust the reset order in the Shutdown function.

ryanofsky
ryanofsky approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 99a668c

Changes look good, but I would suggest changing the PR title to something like: "shutdown: destroy kernel last and make test shutdown order consistent" because the current title "tests: Reset node context members on ~ChainTestingSetup" makes it seems like this is a test-only change.

Also think it would be better to split changes in init.cpp and setup_common.cpp into separate commits. The changes are both related to shutdown but they have different rationales and effects, so it doesn't really make sense for them to be in the same commit.

@sedited sedited changed the title tests: Reset node context members on ~ChainTestingSetup shutdown: Destroy kernel last, make test shutdown order consistent Oct 24, 2023
@sedited
shutdown: Destroy kernel last
9759af1 
Currently the shutdown function resets the kernel before the
chainman and scheduler. Invert this order by resetting the kernel
last, since they might rely on the kernel.
@sedited
tests: Reset node context members on ~BasicTestingSetup
c1144f0 
The destruction/resetting of node context members in the tests should
roughly follow the behaviour of the Shutdown function in `init.cpp`.
@sedited sedited force-pushed the testSetupDestructorOrder branch from 99a668c to c1144f0 Compare October 24, 2023 06:42
@sedited
Copy link
Contributor Author

sedited commented Oct 24, 2023

Updated 99a668c -> c1144f0 (testSetupDestructorOrder_1 -> testSetupDestructorOrder_2, compare)

@maflcko maflcko removed the Tests label Oct 24, 2023
ryanofsky
ryanofsky approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK c1144f0. No code changes since last review, just updated commits and descriptions

@maflcko
Copy link
Member

maflcko commented Oct 25, 2023

ACK c1144f0 🗣

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK c1144f0076339c775f41d4b5fcfdc72191440d96 🗣
T9dJ5U2qRZJzAofSxNNeOCDY1x9g1v2UzSnLfzGQ+Ge86LGQmtA47PGOhyV4baDkH4joyvRrBHb2K0taFvI/Dw==

@achow101
Copy link
Member

achow101 commented Nov 7, 2023

ACK c1144f0

@achow101 achow101 merged commit c981771 into bitcoin:master Nov 7, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@theuni theuni Awaiting requested review from theuni

Assignees

@ryanofsky ryanofsky

Labels

None yet

Projects

The libbitcoinkernel Project
Status: Done or Closed or Rethinking

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sedited @DrahtBot @maflcko @achow101 @ryanofsky