Added support for persistent connections #30

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants
@clintongormley
Contributor

clintongormley commented Oct 4, 2013

I've followed the ideas in issue #14 and implemented persistent connections.Sockets are reused if:

  • keep_alive is true
  • the host, port, scheme, and timeout match the values of the previous connection
  • the socket isn't dirty ( $self->can_read == false)
  • the socket hasn't been closed

To give you an idea of what a difference it makes when using it to talk to Elasticsearch (HTTPTiny2 uses persistent connections):

    Write:
              s/iter  HTTPTiny HTTPTiny2
    HTTPTiny    1.77        --      -46%
    HTTPTiny2  0.956       86%        --

    Read:
              s/iter  HTTPTiny HTTPTiny2
    HTTPTiny    1.50        --      -43%
    HTTPTiny2  0.856       75%        --

I WANT this :)

The bit I'm missing is how to write the tests for it. Any pointers?

Added support for persistent connections
If keep_alive => 1, then HTTP::Tiny will try to reuse the previous socket,
but only for the same scheme/host/port/timeout. Any other changes will
use a new socket
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 4, 2013

Hi, Clint. The tests don't pass because of "HTTP::Tiny::Handle2" not found. Could you please clean it up and make sure tests are passing first?

Meanwhile, I'll take a look at the delta and see if I see any issues. It's a nice speed-up for single-host activities.

ghost commented Oct 4, 2013

Hi, Clint. The tests don't pass because of "HTTP::Tiny::Handle2" not found. Could you please clean it up and make sure tests are passing first?

Meanwhile, I'll take a look at the delta and see if I see any issues. It's a nice speed-up for single-host activities.

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Oct 4, 2013

Contributor

@dagolden Sorry - copied-pasted. Embarrassingly, it didn't even occur to me to run the test suite before submitting the PR :(

Contributor

clintongormley commented Oct 4, 2013

@dagolden Sorry - copied-pasted. Embarrassingly, it didn't even occur to me to run the test suite before submitting the PR :(

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 4, 2013

I also don't see anything addressing the RFC 2616 guidelines on ensuring a content length: https://tools.ietf.org/html/rfc2616#section-8.1.2.1

I'm not sure we always do, so you should check that as well.

ghost commented Oct 4, 2013

I also don't see anything addressing the RFC 2616 guidelines on ensuring a content length: https://tools.ietf.org/html/rfc2616#section-8.1.2.1

I'm not sure we always do, so you should check that as well.

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Oct 4, 2013

Contributor

OK - I can add that in. Which looks like I should disable persistence for chunked bodies too.

(sorry, as you know I'm no expert, I just adapted it from LWP::UserAgent)

Contributor

clintongormley commented Oct 4, 2013

OK - I can add that in. Which looks like I should disable persistence for chunked bodies too.

(sorry, as you know I'm no expert, I just adapted it from LWP::UserAgent)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 4, 2013

There's also the part about recovering from asynchronous close: https://tools.ietf.org/html/rfc2616#section-8.1.4

I'm not sure checking can_read before the request is enough, though that in combination with our for (0 .. 1) loop in request might be.

ghost commented Oct 4, 2013

There's also the part about recovering from asynchronous close: https://tools.ietf.org/html/rfc2616#section-8.1.4

I'm not sure checking can_read before the request is enough, though that in combination with our for (0 .. 1) loop in request might be.

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Oct 4, 2013

Contributor

OK - I'm calling it a night here, but I'll look at this again as soon as I can.

Also, would you mind giving me a 2 min class in the approach i should take in writing tests for this? Just point me at the right methods/subs and good examples of what i need to do, and i'll figure out the rest

Contributor

clintongormley commented Oct 4, 2013

OK - I'm calling it a night here, but I'll look at this again as soon as I can.

Also, would you mind giving me a 2 min class in the approach i should take in writing tests for this? Just point me at the right methods/subs and good examples of what i need to do, and i'll figure out the rest

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 4, 2013

@clintongormley chunked should be OK because that has a protocol for determining the end. But unless we're using that, we have to set a content length.

But, hey, thank you for getting this ball rolling. I think this particular single host/port/etc/ approach is conceptually "Tiny" and I feel pretty confident.

ghost commented Oct 4, 2013

@clintongormley chunked should be OK because that has a protocol for determining the end. But unless we're using that, we have to set a content length.

But, hey, thank you for getting this ball rolling. I think this particular single host/port/etc/ approach is conceptually "Tiny" and I feel pretty confident.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 4, 2013

I'm not sure there's a real test you can write for this because we monkeypatch HTTP::Tiny::Handle in t/Util.pm.

I think the test is probably to check $ua->{handle} before/after a request and see that it's populated. Possibly test to see that the handle is getting reused. Maybe by having the monkey patch routines keep a count of the number of requests on the handle... That's my brainstorm so far. :-)

ghost commented Oct 4, 2013

I'm not sure there's a real test you can write for this because we monkeypatch HTTP::Tiny::Handle in t/Util.pm.

I think the test is probably to check $ua->{handle} before/after a request and see that it's populated. Possibly test to see that the handle is getting reused. Maybe by having the monkey patch routines keep a count of the number of requests on the handle... That's my brainstorm so far. :-)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 4, 2013

The tests are a bit hairy, but in short, t/cases contain a bunch of test cases with multiple parts:

  • a preamble with variables to control the test
  • the expected request
  • a response to return
  • ... possibly more request/response pairs ...

So a test file is often "grab matching case files, parse them, run them". Walking through, say, t/130_redirect.t should give you a sense of it.

ghost commented Oct 4, 2013

The tests are a bit hairy, but in short, t/cases contain a bunch of test cases with multiple parts:

  • a preamble with variables to control the test
  • the expected request
  • a response to return
  • ... possibly more request/response pairs ...

So a test file is often "grab matching case files, parse them, run them". Walking through, say, t/130_redirect.t should give you a sense of it.

clintongormley added some commits Oct 5, 2013

Don't reuse the connection if the content-length header is missing or
incorrect. Also, uncache the handle at the start of the request and
only recache it on success, in case anything else dies along the way
@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Oct 5, 2013

Contributor

@dagolden I drop the connection now if the content-length header is missing (unless chunked encoding has been specified) or if the content-length is incorrect.

Also, if there is anything left in the handler's read buffer, just in case! I considered turning keep-all on by default, as that's what the RFC says SHOULD happen, but it broke too many tests, I think because they're written to not expect reuse and don't reflect reality.

I now remove the cached handle at the beginning of the request, and only recache it at the end, if everything has succeeded. This way we avoid reuse if we have to restart the request for any reason.

I think that's the lot now, but let me know if I've missed anything

Contributor

clintongormley commented Oct 5, 2013

@dagolden I drop the connection now if the content-length header is missing (unless chunked encoding has been specified) or if the content-length is incorrect.

Also, if there is anything left in the handler's read buffer, just in case! I considered turning keep-all on by default, as that's what the RFC says SHOULD happen, but it broke too many tests, I think because they're written to not expect reuse and don't reflect reality.

I now remove the cached handle at the beginning of the request, and only recache it at the end, if everything has succeeded. This way we avoid reuse if we have to restart the request for any reason.

I think that's the lot now, but let me know if I've missed anything

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 5, 2013

Thanks. I'll look at it next week (Tuesday+).

ghost commented Oct 5, 2013

Thanks. I'll look at it next week (Tuesday+).

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Oct 5, 2013

Contributor

Just pushed again - realised I wasn't testing the right thing. Next week is cool, I'm away all week anyway
thanks for looking at this

Contributor

clintongormley commented Oct 5, 2013

Just pushed again - realised I wasn't testing the right thing. Next week is cool, I'm away all week anyway
thanks for looking at this

Clear the cached handle if any attributes changes, not just timeout
Many r/w attributes are taken into account only at connection time, so
if the user sets them, we should clear the cached handle.
@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Oct 13, 2013

Contributor

Hiya @dagolden

Did you have a chance to look at this? I see the note you've added about HTTP::Tiny::UA, but I think persistence is probably Tiny enough and useful enough that it should be in this module, no?

clint

Contributor

clintongormley commented Oct 13, 2013

Hiya @dagolden

Did you have a chance to look at this? I see the note you've added about HTTP::Tiny::UA, but I think persistence is probably Tiny enough and useful enough that it should be in this module, no?

clint

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 13, 2013

Christian and I have agreed that we want single-server persistence in HTTP::Tiny, but no, I haven't had a chance to look at it. I've been swamped this week. Sorry

ghost commented Oct 13, 2013

Christian and I have agreed that we want single-server persistence in HTTP::Tiny, but no, I haven't had a chance to look at it. I've been swamped this week. Sorry

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Nov 27, 2013

Contributor

Hiya @dagolden

Just your friendly monthly ping about this issue :)

clint

Contributor

clintongormley commented Nov 27, 2013

Hiya @dagolden

Just your friendly monthly ping about this issue :)

clint

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 27, 2013

Hi, Clint. I appreciate the reminder. It's on my list and with Perl code
freeze coming in Feb, there is a deadline that I'd like to hit, so thank
you for pinging.

David

On Wed, Nov 27, 2013 at 8:19 AM, Clinton Gormley
notifications@github.comwrote:

Hiya @dagolden https://github.com/dagolden

Just your friendly monthly ping about this issue :)

clint


Reply to this email directly or view it on GitHubhttps://github.com/chansen/p5-http-tiny/pull/30#issuecomment-29383424
.

David Golden xdg@xdg.me
Take back your inbox!http://www.bunchmail.com/
Twitter/IRC: @xdg

ghost commented Nov 27, 2013

Hi, Clint. I appreciate the reminder. It's on my list and with Perl code
freeze coming in Feb, there is a deadline that I'd like to hit, so thank
you for pinging.

David

On Wed, Nov 27, 2013 at 8:19 AM, Clinton Gormley
notifications@github.comwrote:

Hiya @dagolden https://github.com/dagolden

Just your friendly monthly ping about this issue :)

clint


Reply to this email directly or view it on GitHubhttps://github.com/chansen/p5-http-tiny/pull/30#issuecomment-29383424
.

David Golden xdg@xdg.me
Take back your inbox!http://www.bunchmail.com/
Twitter/IRC: @xdg

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Jan 25, 2014

Contributor

Hiya David.

I see the Perl code freeze is just around the corner, but I haven't seen any movement on this issue. I'd dearly love to see this released as it will mean that I can use HTTP::Tiny as the default backed for my Elasticsearch API instead of the slower LWP. Hence the re-ping :)

clint

Contributor

clintongormley commented Jan 25, 2014

Hiya David.

I see the Perl code freeze is just around the corner, but I haven't seen any movement on this issue. I'd dearly love to see this released as it will mean that I can use HTTP::Tiny as the default backed for my Elasticsearch API instead of the slower LWP. Hence the re-ping :)

clint

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 25, 2014

Hi, Clint. I still haven't found tuits for this. I suspect it won't make the Feb 20 code freeze, but whenever it's done it can always be released to CPAN.

ghost commented Jan 25, 2014

Hi, Clint. I still haven't found tuits for this. I suspect it won't make the Feb 20 code freeze, but whenever it's done it can always be released to CPAN.

@timbunce

This comment has been minimized.

Show comment
Hide comment
@timbunce

timbunce Jan 27, 2014

Hi @dagolden, just a note that I'd like to see this get in soonish if possible.
(And thanks @clintongormley for working on it.)

Hi @dagolden, just a note that I'd like to see this get in soonish if possible.
(And thanks @clintongormley for working on it.)

@dagolden dagolden referenced this pull request Feb 4, 2014

Closed

Future of HTTP::Tiny #28

@mjegh

This comment has been minimized.

Show comment
Hide comment
@mjegh

mjegh Feb 15, 2014

Clinton's patch is really worth it as it does speed HTTP::Tiny up a lot. However, due to HTTP::Tiny writing a GET request in 2 parts (2 calls to write) it suffers from the Nagle algorithm. I wrote it all up at http://www.martin-evans.me.uk/node/169 which also contains a small patch to get rid of the double write.

Thank you for writing HTTP::Tiny and can I add my vote for what it is worth to encourage you to pull these changes in. If you want a pull request instead of the patch just let me know.

mjegh commented Feb 15, 2014

Clinton's patch is really worth it as it does speed HTTP::Tiny up a lot. However, due to HTTP::Tiny writing a GET request in 2 parts (2 calls to write) it suffers from the Nagle algorithm. I wrote it all up at http://www.martin-evans.me.uk/node/169 which also contains a small patch to get rid of the double write.

Thank you for writing HTTP::Tiny and can I add my vote for what it is worth to encourage you to pull these changes in. If you want a pull request instead of the patch just let me know.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 15, 2014

Worst case, I'm planning on using the QA Hackathon to review the patch in detail.

A PR for your patch would be great!

ghost commented Feb 15, 2014

Worst case, I'm planning on using the QA Hackathon to review the patch in detail.

A PR for your patch would be great!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 17, 2014

This has been incorporated into the devel branch

ghost commented Feb 17, 2014

This has been incorporated into the devel branch

@dagolden dagolden closed this Feb 17, 2014

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