rpc: fix -rpcclienttimeout 0 option #17131

fjahr commented at 10:32 PM on October 13, 2019: member

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.

DrahtBot added the label RPC/REST/ZMQ on Oct 14, 2019

hebasto commented at 7:02 AM on October 14, 2019: member

@fjahr Thank you

... if 0 is given as an argument, the fallback that is being used is the libevent default of 50 seconds...

Is it documented somewhere?

laanwj commented at 9:49 AM on October 14, 2019: member

Actually adding the feature as described in the help string

It would be nice if it actually worked. For long operations (such as getutxosetinfo) it can be undesirable to have a timeout at all.

Then again, ACK 6810956191f8dd26e918c6991f95dbc1a05af3e9 on fixing the documentation first. It can always be re-added.

laanwj added the label Needs backport (0.19) on Oct 14, 2019

MarcoFalke commented at 12:36 PM on October 14, 2019: member

According to #9903 (comment) it used to work at some point

fjahr commented at 9:56 PM on October 14, 2019: member

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.

According to #9903 (comment) it used to work at some point

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.

laanwj commented at 7:05 AM on October 15, 2019: member

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 0 is provided.

hebasto commented at 7:22 AM on October 15, 2019: member

Is it documented somewhere?

https://github.com/libevent/libevent/blob/dda8968c71f684235abb3cf6c26810751bf2c31a/http-internal.h#L19

#define HTTP_READ_TIMEOUT	50

https://github.com/libevent/libevent/blob/dda8968c71f684235abb3cf6c26810751bf2c31a/include/event2/http.h#L601-L604

 * Connection also has default timeouts for the following events:
 * - connect HTTP_CONNECT_TIMEOUT, which is 45 seconds
 * - read    HTTP_READ_TIMEOUT which is 50 seconds
 * - write   HTTP_WRITE_TIMEOUT, which is 50 seconds

fjahr force-pushed on Oct 15, 2019

fjahr commented at 12:26 PM on October 15, 2019: member

Changed to fix the feature by setting timeout to 5 years when 0 is passed to -rpcclienttimeout.

in src/bitcoin-cli.cpp:325 in dff1648064 outdated

 321 | +    // Set connection timeout
 322 | +    int timeout = gArgs.GetArg("-rpcclienttimeout", DEFAULT_HTTP_CLIENT_TIMEOUT);
 323 | +    if (timeout > 0) {
 324 | +        evhttp_connection_set_timeout(evcon.get(), timeout);
 325 | +    } else {
 326 | +        const int YEAR_IN_SECONDS = 31556952; // Average length of year in Gregorian calendar

hebasto commented at 12:30 PM on October 15, 2019:

        constexpr int YEAR_IN_SECONDS = 31556952; // Average length of year in Gregorian calendar

laanwj commented at 12:33 PM on October 15, 2019:

maybe add a comment that this is a workaround for libevent-http having no way to set an infinite timeout

fjahr commented at 4:02 PM on October 15, 2019:

done

fjahr commented at 4:02 PM on October 15, 2019:

done

in src/bitcoin-cli.cpp:326 in dff1648064 outdated

 322 | +    int timeout = gArgs.GetArg("-rpcclienttimeout", DEFAULT_HTTP_CLIENT_TIMEOUT);
 323 | +    if (timeout > 0) {
 324 | +        evhttp_connection_set_timeout(evcon.get(), timeout);
 325 | +    } else {
 326 | +        const int YEAR_IN_SECONDS = 31556952; // Average length of year in Gregorian calendar
 327 | +        evhttp_connection_set_timeout(evcon.get(), 5*YEAR_IN_SECONDS);

hebasto commented at 12:30 PM on October 15, 2019:

        evhttp_connection_set_timeout(evcon.get(), 5 * YEAR_IN_SECONDS);

fjahr commented at 4:02 PM on October 15, 2019:

done

in src/bitcoin-cli.cpp:321 in dff1648064 outdated

 315 | @@ -316,7 +316,15 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 316 |  
 317 |      // Synchronously look up hostname
 318 |      raii_evhttp_connection evcon = obtain_evhttp_connection_base(base.get(), host, port);
 319 | -    evhttp_connection_set_timeout(evcon.get(), gArgs.GetArg("-rpcclienttimeout", DEFAULT_HTTP_CLIENT_TIMEOUT));
 320 | +
 321 | +    // Set connection timeout
 322 | +    int timeout = gArgs.GetArg("-rpcclienttimeout", DEFAULT_HTTP_CLIENT_TIMEOUT);

hebasto commented at 12:33 PM on October 15, 2019:

    const int timeout = gArgs.GetArg("-rpcclienttimeout", DEFAULT_HTTP_CLIENT_TIMEOUT);

fjahr commented at 4:02 PM on October 15, 2019:

done

hebasto changes_requested

hebasto commented at 12:34 PM on October 15, 2019: member

Could limit the scope of timeout by placing into a block?

rpc: fix -rpcclienttimeout 0 option b3b26e149c

fjahr force-pushed on Oct 15, 2019

fjahr commented at 4:04 PM on October 15, 2019: member

Added comment hint to libevent work-around and applied improvement suggestions from @hebasto

MarcoFalke commented at 4:06 PM on October 15, 2019: member

unsigned ACK b3b26e149c34fee9c7ae8548c6e547ec6254b441

fjahr renamed this:
~~rpc: fix -rpcclienttimeout help string~~
rpc: fix -rpcclienttimeout 0 option
on Oct 15, 2019

fanquake deleted a comment on Oct 15, 2019

fanquake requested review from laanwj on Oct 15, 2019

laanwj referenced this in commit 5a3dd93594 on Oct 16, 2019

laanwj merged this on Oct 16, 2019

laanwj closed this on Oct 16, 2019

fanquake referenced this in commit e1bacb591a on Oct 19, 2019

fanquake removed the label Needs backport (0.19) on Oct 19, 2019

laanwj referenced this in commit 5b68d1654f on Oct 21, 2019

HashUnlimited referenced this in commit 15a80b9f19 on Nov 17, 2019

fxtc referenced this in commit 0844853a52 on Nov 25, 2019

fxtc referenced this in commit 054f10742f on Nov 25, 2019

jasonbcox referenced this in commit cf81f8136e on Oct 25, 2020

PastaPastaPasta referenced this in commit 7b76b71465 on Jun 27, 2021

PastaPastaPasta referenced this in commit 6475742158 on Jun 28, 2021

PastaPastaPasta referenced this in commit e17f279de3 on Jun 29, 2021

PastaPastaPasta referenced this in commit 45cae92cfd on Jul 1, 2021

PastaPastaPasta referenced this in commit b7b22c59c7 on Jul 1, 2021

PastaPastaPasta referenced this in commit 359577f505 on Jul 12, 2021

PastaPastaPasta referenced this in commit 66fce80422 on Jul 13, 2021

PastaPastaPasta referenced this in commit bd18cd8e07 on Jul 13, 2021

Munkybooty referenced this in commit 6ce7e306b1 on Dec 9, 2021

DrahtBot locked this on Dec 16, 2021