-
Notifications
You must be signed in to change notification settings - Fork 629
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
retry _call if connection times out #128
base: master
Are you sure you want to change the base?
Conversation
@petertodd Bitcoin closes its connection after about 30s and python-bitcoinlib doesn't notice. For users of python-bitcoinlib who need a long-running connection to Bitcoin, this patch automatically retries, so handles connection drops. |
Note that you're adding another dependency here, the retrying library. Can this be rewritten to not have that dependency? Also, are we 100% sure that this won't cause us to also retry even if the call actually succeeded? For instance, imagine a sendpayment RPC call is in fact successfully sent to bitcoind, but the connection drops before the reply gets back. |
Hey Peter, Yes, the instance you mentioned seems like it would retry even if the call Best, On Sat, Nov 19, 2016 at 1:28 PM, Peter Todd [email protected]
|
I would not recommend using https://botbot.me/freenode/bitcoin-core-dev/2016-11-21/?msg=76819688&page=2 |
Well the real bug is that bitcoind closes the connection and python-bitcoinlib doesn't notice. The next time the client tries to make an rpc call, it throws an exception. As far as I'm concerned this is a bug to pass responsibility for reopening connections to the caller. @stefanlenoach what do you think about: Upon proxy.call, check if the connection is open. If not, open a new one and issue the call. This avoids potentially problematic retrying, but won't handle flaky connections -- if the connection is terminated in an unusual manner, it's still the caller's logic that decides whether to resubmit the transaction, or re-request data. The case we're concerned about here is not an unusual close, it's the regular and well behaved close by bitcoind when it times out. |
@mcelrath I think your suggestion will work well enough for our On Mon, Nov 21, 2016 at 4:56 PM, Bob McElrath [email protected]
|
Check here, it looks like this was fixed in bitcoin core: https://github.com/bitcoin/bitcoin/blob/master/qa/rpc-tests/test_framework/authproxy.py#L134 |
We're still having a heck of a time with keeping the RPC connection to bitcoind active. Here is yet another solution that uses The behavior I observe is that when bitcoind closes the connection, I get a |
@petertodd We have a more complete solution to this here, using epoll, if the system supports it. This is effective and doesn't incur the problem of possibly sending transactions twice. |
Having hit this problem AGAIN because epoll is not supported on macosx, it seems clear to me that the right answer is to rewrite this patch to use aiohttp, and catch |
we just substituted connection object in BaseProxy with our own wrapper class, that handles reconnections itself. retrying bitcoind RPC calls is not an issue for us, because we use it only to get transaction data and send raw transactions. Of course using auto-reconnecting connection with sendtoaddress() and the like is a bad idea. |
I have forward-ported this patch to python-bitcoinlib 0.11:
I am unsure of the status of this patch and have not tested it yet. I've lost the development stack we were using when it was originally created and no longer have write access to the repository from which this PR is sourced. I think they've stopped using bitcoin entirely and the PR is stale. @stefanlenoach if you're still working there can you comment on the disposition of this PR? I'll close and reopen this PR if people agree EPOLL is a good direction. So is EPOLL a good direction... I'm not a fan of closing and re-opening connections as @petertodd mentions above, this can lead to unexpected behavior, and requests getting dropped in between. It appears that the current code does a retry and reopen if the connection is dropped, which is the approach @petertodd disagreed with above (and I agree with him). You REALLY don't want to use So this needs further testing. I'm not 100% certain of the behavior of the current code WRT retries, but I don't like retries. I think if EPOLL works and is superior, the suggestion in this comment is probably a way to move this forward. But for now, I forward ported it in case anyone else wants to test. |
No description provided.