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

Buggy keep-alive implementation #98

Closed
TJC opened this Issue Nov 28, 2016 · 13 comments

Comments

Projects
None yet
3 participants
@TJC
Copy link

TJC commented Nov 28, 2016

I find Furl's keep-alive implementation to cause problems.

I have an HTTP server that does implement keep-alive, but will close the connection after a period of idle time.

If I use Furl to make a request, then delay too long, then try again, Furl fails with "Connection reset by peer" error.

ie. This fails:

my $furl = Furl->new;
$furl->get($url);
sleep 120;
$furl->get($url);

but this succeeds:

Furl->new->get($url);
sleep 120;
Furl->new->get($url);
@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

I have created a complete demonstration of this issue, to assist you. Please see this github repo:
https://github.com/TJC/Furl-bug-demo

It contains an HTTP server with a short keep-alive timeout set (5 seconds) and a script to call Furl with gradually increasing delays between calls.

Output of test script:

$ ./furlTest.pl
GET #1 successful.
Sleeping 1 sec
GET #2 successful.
Sleeping 3 sec
GET #3 successful.
Sleeping 6 sec
GET #4 failed. at ./furlTest.pl line 26.
@tokuhirom

This comment has been minimized.

Copy link
Owner

tokuhirom commented Nov 28, 2016

It's spec.

@tokuhirom tokuhirom closed this Nov 28, 2016

@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

I spent a long time tracking down this issue in my code, and creating an easily reproducible test case.
I am disappointed that you closed this issue with just two words.

Could you spend a couple of minutes more and write an explanation?

@kazuho

This comment has been minimized.

Copy link
Collaborator

kazuho commented Nov 28, 2016

As long as you use keep-alive, there is an inherent race condition that leads to a HTTP request being lost. Cconsider the case when a server closes the connection at the same moment when the client sends a request.

It is the application's responsibility to replay such requests.

@kazuho

This comment has been minimized.

Copy link
Collaborator

kazuho commented Nov 28, 2016

PS. note that due to how TCP is designed, there is no reliable way to detect if the server closed the connection after receiving the request or if it closed the connection before receiving it. Therefore, the application (that knows if a request is idempotent) should either replay the request, or, refrain from using a persistent connection for requests that are not idempotent.

@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

I note that in the Furl source code, you appear to try and detect ECONNRESET to do a retry.

https://metacpan.org/source/SYOHEX/Furl-3.09/lib/Furl/HTTP.pm#L464

Is that code broken, or am I misunderstanding its purpose?

@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

If I want to disable persistent requests from Furl, how is that achieved?

Is there a keep-alive => 0 flag I can set somewhere? I didn't see one in the documentation.

@kazuho

This comment has been minimized.

Copy link
Collaborator

kazuho commented Nov 28, 2016

I note that in the Furl source code, you appear to try and detect ECONNRESET to do a retry.

It is fairly safe to assume that the server did not receive a full request in case of ECONNRESET (and there would indeed be other cases that you could safely retry).

But there are cases in which it is not safe to retry. In other words, you should always handle retries in your application.

If I want to disable persistent requests from Furl, how is that achieved?

My understanding is that you can do that by addingconnection: close header to the request (but I might be wrong).

@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

For what it's worth, HTTP::Tiny supports keep-alive, yet does not suffer from this issue.

And I've now tested LWP::UserAgent too.. it's also fine.

HTTP Tiny's source code notes that:
# RFC 2616 Section 8.1.4 mandates a single retry on broken socket

@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

Thanks for your comments, Kazuho.

I can see your reasoning for suggesting that clients should manage their own retries; however I can also see that many end-users will not realise they need to do so. Especially if they are coming from one of the other HTTP libraries that handles the retry automatically.

Maybe something in the documentation would be helpful?

@kazuho

This comment has been minimized.

Copy link
Collaborator

kazuho commented Nov 28, 2016

# RFC 2616 Section 8.1.4 mandates a single retry on broken socket

That is untrue. I'd suggest referring to RFC 7230 (that updated 2616), but neither of them allows any HTTP request to be retried.

To be precise, only requests using idempotent methods can be retried. However, for an HTTP library that accepts the method to be used from the application, it is impossible to determine whether if a method is idempotent, since other specifications (and users) are allowed to define new methods.

Therefore, it is reasonable to let the application determine whether if it can retry the request.

For what it's worth, HTTP::Tiny supports keep-alive, yet does not suffer from this issue.

It could be the case that HTTP::Tiny does more than Furl to determine if a request can be retried. However, as stated, there is no way for an HTTP library to perfectly assess whether if a request should be retried or not. In other words, if you are using an HTTP library conformant to the RFC, then you need to be careful of handling retries.

I'd say that you were lucky to find the issue in your application by using Furl :-)

Maybe something in the documentation would be helpful?

Yes, I think that a PR to the document might be helpful in that respect.

@TJC

This comment has been minimized.

Copy link

TJC commented Nov 28, 2016

I mentioned before that the Furl code appears to try and retry upon ECONNRESET, at:
https://metacpan.org/source/SYOHEX/Furl-3.09/lib/Furl/HTTP.pm#L464

Yet the demonstration code I presented to you fails on a GET with Connection Reset:
500 Internal Response: Failed to send HTTP request: Connection reset by peer

Shouldn't that be picked up by the code around line 464 of Furl/HTTP.pm?

@kazuho

This comment has been minimized.

Copy link
Collaborator

kazuho commented Nov 29, 2016

I mentioned before that the Furl code appears to try and retry upon ECONNRESET, at:
https://metacpan.org/source/SYOHEX/Furl-3.09/lib/Furl/HTTP.pm#L464

Considering the fact that you are seeing Furl report: Failed to send HTTP request:, I believe the part of the code that is returning the error is https://metacpan.org/source/SYOHEX/Furl-3.09/lib/Furl/HTTP.pm#L403-405.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment