Skip to content

cln-grpc, clnrest: workaround for logging before shutdown#8085

Merged
endothermicdev merged 1 commit intoElementsProject:masterfrom
daywalker90:cln-log-shutdown-workaround
Feb 14, 2025
Merged

cln-grpc, clnrest: workaround for logging before shutdown#8085
endothermicdev merged 1 commit intoElementsProject:masterfrom
daywalker90:cln-log-shutdown-workaround

Conversation

@daywalker90
Copy link
Collaborator

@daywalker90 daywalker90 commented Feb 12, 2025

Changelog-Fixed: cln-grpc and clnrest errors that cause them to stop will now log

Important

25.02 FREEZE JANUARY 31ST: Non-bugfix PRs not ready by this date will wait for 25.05.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

I consider this a bug fix pr for 25.02

@daywalker90 daywalker90 requested a review from cdecker as a code owner February 12, 2025 16:16
@endothermicdev endothermicdev added this to the v25.02 milestone Feb 12, 2025
@daywalker90 daywalker90 force-pushed the cln-log-shutdown-workaround branch from 2bb9396 to c7b81c4 Compare February 12, 2025 17:42
@endothermicdev
Copy link
Collaborator

I don't think this failure was related to the PR, but as it's dealing with cln-rest, it's slightly suspicious. Let's allow a little more time on that test under valgrind.

@daywalker90
Copy link
Collaborator Author

Yeah i cant reproduce that locally, dont know what to do about it

@endothermicdev
Copy link
Collaborator

I tried it with the socket connection timeout set to 0.5s versus the default of 1 and it failed every time (when running under valgrind.) If we're that close, we may as well bump up the tolerance a bit. Under normal testing everything is snappy and I don't see any failures.

@endothermicdev endothermicdev force-pushed the cln-log-shutdown-workaround branch from 531271f to 95e8be0 Compare February 13, 2025 21:36
@daywalker90
Copy link
Collaborator Author

If that is the problem then from my experience the github ci is so slow that you need to up that number to 10 or something^^

@rustyrussell
Copy link
Contributor

taskset -c 1 can sometimes slow your own machine down to GH speeds :)

Also see #8090

Changelog-Fixed: cln-grpc and clnrest errors that cause them to stop will now log
@endothermicdev
Copy link
Collaborator

Rebasing this onto master (don't mix Valgrind & Rust.)

@endothermicdev endothermicdev force-pushed the cln-log-shutdown-workaround branch from 95e8be0 to e0f2b60 Compare February 14, 2025 02:53
@endothermicdev endothermicdev merged commit 5e0a25b into ElementsProject:master Feb 14, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants