search

chansen / p5-http-tiny

Tiny HTTP Client
https://metacpan.org/dist/HTTP-Tiny
53 stars 52 forks source link

Don't reuse closed connections #117

Closed guimard closed 3 years ago

guimard commented 6 years ago

Sometimes, a closed connection isn't detected by HTTP::Tiny. To reproduce the bug, use feersum sources:

$ perl Makefile.PL && make
$ taskset -c 0 perl -I blib/lib -I blib/arch t/63*

HTTP::Tiny tries to reuse a closed connection (it looks like Feersum closes keep-alive connection on heavy load. A separate issue is opened for this).

This little patch fixes the problem.

xdg commented 6 years ago

I'm concerned this is insufficient. The note on connected says:

Note: This method cannot reliably be used to discover whether the remote host has closed its end of the socket.

That said, I don't think it will hurt to apply.

I wonder if we need to select-poll for writeability (i.e. call can_write with 0 timeout).

@chansen, thoughts?

guimard commented 6 years ago

Hello @xdg,

Thanks for your reply. I don't know if this is insufficient, but this patch fixes the problem for me (discovered by Debian continuous integration: see #909480 thread).

Cheers, Xavier

xdg commented 6 years ago

Looking at the docs, connected will detect a full close, but not a half close, sometimes called "graceful close" where the remote shuts its output side and waits for us to do the same.

A solution is suggested by this article, which is to check if the socket is readable and then to peek at the socket with recv with MSG_PEEK | MSG_DONTWAIT flags.

We could do that, though a simpler option could be just to check readability without the subsequent peek. Given the synchronous nature of the protocol, a socket we're going to use for writing should have had all data read from it, thus it should not be readable, except in the situation where a server has closed its outbound side. The only reason I can see for not using MSG_PEEK would be portability.

guimard commented 5 years ago

Do you want that I fill a bug for this problem ?

davewood commented 4 years ago

I just stopped by to say that I hope this issue will be fixed.

xdg commented 3 years ago

Sorry to take so long to reconsider this issue. For this use case, if the server has closed the connection, then I agree that connected will detect it. At the point at which the check occurs, half-open connections shouldn't exist.