rpc: fix -rpcclienttimeout 0 option #17131
Conversation
|
@fjahr Thank you
Is it documented somewhere? |
It would be nice if it actually worked. For long operations (such as Then again, ACK 6810956191f8dd26e918c6991f95dbc1a05af3e9 on fixing the documentation first. |
|
According to #9903 (comment) it used to work at some point |
|
Unfortunately, there is nothing on this in the docs and due to this issue of libevent I guess they were not aware of this behavior either. This code is now changed in master but not in any release, here it is in the code of 2.1.11 which is the latest stable release. If I understand the code correctly the timeout values are always initialized as 0 and then overridden with the defaults if they are still 0 at the end, so setting them explicitly to 0 does nothing. Since nothing has happened to their issue it seems there is actually no way to set no timeout on libevent right now. I don't see a simple way to make long-running connections work so I would suggest merging the fixed help text and then thinking about how we could add it. But maybe we set the timeout to our default (900) if 0 is passed? Also not ideal but makes a little more sense than the 'magic' 50 seconds.
Hm, I am really not sure what happened there. Our code hasn't changed and I don't see how libevent could have behaved differently either. |
|
I also have a vague recollection that it used to work at some point. As a workaround I guess we could change it to give libevent an absurd timeout of say, four years if the value |
|
fjahr
force-pushed
the
rpcclienttimeout
branch
from
6810956 to
dff1648
Compare
|
Changed to fix the feature by setting timeout to 5 years when 0 is passed to |
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
Could limit the scope of timeout by placing into a block?
fjahr
force-pushed
the
rpcclienttimeout
branch
from
dff1648 to
b3b26e1
Compare
|
Added comment hint to |
|
unsigned ACK b3b26e1 |
fjahr
changed the title
b3b26e1 rpc: fix -rpcclienttimeout 0 option (Fabian Jahr) Pull request description: fixes #17117 I understood the bug as the help string being wrong, rather than that this feature is missing and should be added. Let me know if it should be the other way around. It is notable that if 0 is given as an argument, the fallback that is being used is the libevent default of 50 seconds, rather than `DEFAULT_HTTP_CLIENT_TIMEOUT` (900 seconds). This is not intuitive for the user. I could handle this in this PR but I am unsure which would be the better solution then: Actually adding the feature as described in the help string or falling back to `DEFAULT_HTTP_CLIENT_TIMEOUT`? Happy to hear opinions. ACKs for top commit: MarcoFalke: unsigned ACK b3b26e1 Tree-SHA512: 65e526a652c0adcdb4f895e8d78d60c7caa5904c9915b165a3ae95725c87d13af1f916359f80302452a2fcac1a80f4c58cd805ec8c28720fa4b91b3c8baa4155
Github-Pull: bitcoin#17131 Rebased-From: b3b26e1
334e27e util: Filter out macOS process serial number (Hennadii Stepanov) e1bacb5 rpc: fix -rpcclienttimeout 0 option (Fabian Jahr) 6a45766 doc: update bips.md with buried BIP9 deployments (MarcoFalke) dc0fe7a util: Filter control characters out of log messages (Wladimir J. van der Laan) ba46f39 init: Change fallback locale to C.UTF-8 (Wladimir J. van der Laan) Pull request description: Backports the following PRs to the `0.19.0` [branch](https://github.com/bitcoin/bitcoin/tree/0.19): * #17184 - util: Filter out macOS process serial number * #17131 - rpc: fix -rpcclienttimeout 0 option * #17111 - doc: update bips.md with buried BIP9 deployments * #17095 - util: Filter control characters out of log messages * #17085 - init: Change fallback locale to C.UTF-8 ACKs for top commit: laanwj: ACK 334e27e Tree-SHA512: 436064c00f98bae8475d0e46ab104df6fc9bdae4927dcdd5cffa4242704256c749352e9cabb23cf806911b1c303ddcb0208a42d540412e98da2513176e5e1023
Github-Pull: bitcoin#17131 Rebased-From: b3b26e1
Summary: When `-rpcclienttimeout=0` was provided, the default `libevent` timeout of 50 s was used, instead of no timeout as documented. This change fixes it by setting timeout to 5 years when 0 is passed to `-rpcclienttimeout`. This is a backport of Core [[bitcoin/bitcoin#17131 | PR17131]] Test Plan: ``` $ src/bitcoin-cli -rpcclienttimeout=10 gettxoutsetinfo error: Could not connect to the server 127.0.0.1:8332 (error code 0 - "timeout reached") $ time src/bitcoin-cli -rpcclienttimeout=0 gettxoutsetinfo { "height": 658626, "bestblock": "000000000000000002050eb8ee46cdb7d56cbc816ca13e3de000bee82bde9d34", "transactions": 17556030, "txouts": 37933218, "bogosize": 2871043050, "hash_serialized": "4790608c2e6ef02606f38680d5716b632e6cb006284c7f36c269d73be038496c", "disk_size": 2156465916, "total_amount": 18553754.67451907 } real 1m52,365s user 0m0,006s sys 0m0,001s ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8109
b3b26e1 rpc: fix -rpcclienttimeout 0 option (Fabian Jahr) Pull request description: fixes bitcoin#17117 I understood the bug as the help string being wrong, rather than that this feature is missing and should be added. Let me know if it should be the other way around. It is notable that if 0 is given as an argument, the fallback that is being used is the libevent default of 50 seconds, rather than `DEFAULT_HTTP_CLIENT_TIMEOUT` (900 seconds). This is not intuitive for the user. I could handle this in this PR but I am unsure which would be the better solution then: Actually adding the feature as described in the help string or falling back to `DEFAULT_HTTP_CLIENT_TIMEOUT`? Happy to hear opinions. ACKs for top commit: MarcoFalke: unsigned ACK b3b26e1 Tree-SHA512: 65e526a652c0adcdb4f895e8d78d60c7caa5904c9915b165a3ae95725c87d13af1f916359f80302452a2fcac1a80f4c58cd805ec8c28720fa4b91b3c8baa4155
b3b26e1 rpc: fix -rpcclienttimeout 0 option (Fabian Jahr) Pull request description: fixes bitcoin#17117 I understood the bug as the help string being wrong, rather than that this feature is missing and should be added. Let me know if it should be the other way around. It is notable that if 0 is given as an argument, the fallback that is being used is the libevent default of 50 seconds, rather than `DEFAULT_HTTP_CLIENT_TIMEOUT` (900 seconds). This is not intuitive for the user. I could handle this in this PR but I am unsure which would be the better solution then: Actually adding the feature as described in the help string or falling back to `DEFAULT_HTTP_CLIENT_TIMEOUT`? Happy to hear opinions. ACKs for top commit: MarcoFalke: unsigned ACK b3b26e1 Tree-SHA512: 65e526a652c0adcdb4f895e8d78d60c7caa5904c9915b165a3ae95725c87d13af1f916359f80302452a2fcac1a80f4c58cd805ec8c28720fa4b91b3c8baa4155
6 participants
fixes #17117
I understood the bug as the help string being wrong, rather than that this feature is missing and should be added. Let me know if it should be the other way around.
It is notable that if 0 is given as an argument, the fallback that is being used is the libevent default of 50 seconds, rather than
DEFAULT_HTTP_CLIENT_TIMEOUT(900 seconds). This is not intuitive for the user. I could handle this in this PR but I am unsure which would be the better solution then: Actually adding the feature as described in the help string or falling back toDEFAULT_HTTP_CLIENT_TIMEOUT? Happy to hear opinions.