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

CHIA-1645: Some daemon start cleanup #18809

Merged
merged 7 commits into from
Nov 13, 2024
Merged

CHIA-1645: Some daemon start cleanup #18809

merged 7 commits into from
Nov 13, 2024

Conversation

emlowe
Copy link
Contributor

@emlowe emlowe commented Nov 1, 2024

Change the daemon startup procedure:

  • During start, the daemon no longer outputs some JSON on stdout. This was previously read by the GUI (although it's not known how long it was used) - but hasn't been read since earlier this year (see Remove unused code that attempts to read the output of the daemon chia-blockchain-gui#2526) when the GUI startup was changed to use chia start daemon
  • The daemon is launched with setting stdout,stderr,stdin to DEVNULL and with start_new_session
  • Code was added to stop trying to read the daemon stdout as a mechanism to wait for the daemon to be ready and code added to retry to connect to the socket

This seems to work well so far in my testing and I believe this will fix both #18677 and #16396

I tested launching the daemon in a variety of ways including

  • chia start daemon
  • chia run_daemon
  • chia_daemon

So far testing was limited to macOS and Linux, Windows needs some additional testing.

@emlowe emlowe changed the title Some daemon start cleanup CHIA-1645: Some daemon start cleanup Nov 1, 2024
@emlowe emlowe added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Nov 4, 2024
altendky
altendky previously approved these changes Nov 7, 2024
@emlowe
Copy link
Contributor Author

emlowe commented Nov 12, 2024

close and reopen

@emlowe emlowe closed this Nov 12, 2024
@emlowe emlowe reopened this Nov 12, 2024
arvidn
arvidn previously approved these changes Nov 12, 2024
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good! If anything, I would suggest making it mandatory to name the wait_for_start argument. but it's fine either way

Copy link

coveralls-official bot commented Nov 12, 2024

Pull Request Test Coverage Report for Build 11805186875

Details

  • 14 of 18 (77.78%) changed or added relevant lines in 3 files are covered.
  • 44 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.2%) to 90.722%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/cmds/start_funcs.py 0 2 0.0%
chia/daemon/client.py 13 15 86.67%
Files with Coverage Reduction New Missed Lines %
chia/timelord/timelord.py 1 80.0%
chia/_tests/simulation/test_simulation.py 1 96.45%
chia/daemon/keychain_proxy.py 1 74.58%
chia/full_node/full_node_api.py 1 82.27%
chia/farmer/farmer.py 1 72.52%
chia/plotters/madmax.py 1 50.6%
chia/daemon/client.py 1 74.01%
chia/timelord/timelord_launcher.py 2 89.29%
chia/server/ws_connection.py 4 88.06%
chia/full_node/full_node.py 10 86.08%
Totals Coverage Status
Change from base Build 11804208692: -0.2%
Covered Lines: 87097
Relevant Lines: 95827

💛 - Coveralls

@emlowe emlowe dismissed stale reviews from arvidn and altendky via 4701635 November 12, 2024 20:29
Copy link
Contributor

File Coverage Missing Lines
chia/cmds/start_funcs.py 0.0% lines 53-54
chia/daemon/client.py 86.7% lines 57-58
Total Missing Coverage
18 lines 4 lines 77%

@pmaslana pmaslana merged commit ecdf4f4 into main Nov 13, 2024
362 of 363 checks passed
@pmaslana pmaslana deleted the EL.daemon-connect-retry branch November 13, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants