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

Timeout: use less resources, clean them up better and make cancellation deterministic #10254

Merged
merged 10 commits into from
Jan 8, 2025

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Nov 26, 2024

👍 Now we got unit tests!!! ❤️ 144abb1
👍 Now the destructor leads to cancellation. 21fc946
👍 Now the callback is "atomic", i.e. it doesn't yield. 32e64b6 175099c This way cancellation can't happen in a callback yield, making sure once cancelled, the callback won't (continue to) run. Also, we don't need any coroutine which has to carry around shared pointers which prevent own destruction...

closes #10250
closes #10252

@Al2Klimov Al2Klimov added the core/quality Improve code, libraries, algorithms, inline docs label Nov 26, 2024
@cla-bot cla-bot bot added the cla/signed label Nov 26, 2024
yhabteab
yhabteab previously approved these changes Nov 28, 2024
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Please add some documentation to the class. It now has some non-obvious details, especially regarding what happens at destruction (see #10254 (comment)).

@Al2Klimov Al2Klimov force-pushed the Timeout-Cancel branch 2 times, most recently from 983307f to 9074d78 Compare November 29, 2024 13:18
@Al2Klimov Al2Klimov requested a review from yhabteab November 29, 2024 13:21
@Al2Klimov Al2Klimov force-pushed the Timeout-Cancel branch 2 times, most recently from cb3068f to ba63964 Compare November 29, 2024 15:25
yhabteab
yhabteab previously approved these changes Nov 29, 2024
@Al2Klimov Al2Klimov mentioned this pull request Dec 4, 2024
2 tasks
@julianbrost julianbrost dismissed their stale review December 4, 2024 17:30

The requested changes were addressed, so far I didn't get around to do a full review though.

@Al2Klimov
Copy link
Member Author

@yhabteab Please could you have yet another look? After all

  1. I've force-pushed this since Timeout: use less resources, clean them up better and make cancellation deterministic #10254 (review)
  2. We need this if you don't want do exactly copy this PR in JsonRpcConnection: Don't drop client from cache prematurely #10210 (review)

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts! And please drop the Co-Authored-By: entry from bfb0303, that's just so unnecessary!

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

Drop incoming packets on one of the masters port. $ (sudo pfctl -sr 2>/dev/null; echo "block drop in on lo0 proto tcp from any to any port = 5666") | sudo pfctl -e -f - 2>/dev/null

[2025-01-08 10:27:40 +0100] critical/ApiListener: Cannot connect to host 'localhost' on port '5666': Operation canceled [system:89 at /opt/homebrew/include/boost/asio/detail/reactive_socket_connect_op.hpp:97:37 in function 'do_complete']
[2025-01-08 10:27:45 +0100] information/ApiListener: Reconnecting to endpoint 'satellite' via host 'localhost' and port '5666'
...
[2025-01-08 10:28:00 +0100] critical/ApiListener: Timeout while reconnecting to endpoint 'satellite' via host 'localhost' and port '5666', cancelling attempt
[2025-01-08 10:28:00 +0100] critical/ApiListener: Cannot connect to host 'localhost' on port '5666': Operation canceled [system:89 at /opt/homebrew/include/boost/asio/detail/reactive_socket_connect_op.hpp:97:37 in function 'do_complete']
[2025-01-08 10:28:05 +0100] information/ApiListener: Reconnecting to endpoint 'satellite' via host 'localhost' and port '5666'
...
[2025-01-08 10:28:20 +0100] critical/ApiListener: Timeout while reconnecting to endpoint 'satellite' via host 'localhost' and port '5666', cancelling attempt
[2025-01-08 10:43:11 +0100] warning/ApiListener: Timeout while processing incoming connection from [::1]:59149

Reactivate the standard firewall rules again: $ sudo pfctl -f /etc/pf.conf

@yhabteab yhabteab added this to the 2.15.0 milestone Jan 8, 2025
@yhabteab yhabteab added the consider backporting Should be considered for inclusion in a bugfix release label Jan 8, 2025
@julianbrost julianbrost merged commit fba56f0 into master Jan 8, 2025
21 checks passed
@julianbrost julianbrost deleted the Timeout-Cancel branch January 8, 2025 15:28
@Al2Klimov Al2Klimov added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Fix was included in a bugfix release cla/signed core/quality Improve code, libraries, algorithms, inline docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants