Skip to content
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

Added update_timeout API to reconfigure timeouts on the fly #302

Closed
wants to merge 1 commit into from

Conversation

zanker
Copy link
Contributor

@zanker zanker commented Feb 7, 2016

This is silly... and well that's about it, but I can't see a better way of doing it so I think it's known silly? I think this is a legitimate use case, but I can't share the code since it's one of our internal gems.

We have a S2S library, two of the main features are it uses a connection pool + conn reuse, and the other is that you can dynamically reconfigure timeouts. Eg, use a short timeout for primary key lookup APIs but a long timeout for search APIs.

We want to share the same pool between the two because the SSL handshake is expensive. We can't do that with what we have today in HTTP.rb, since we also need to reconfigure the socket when the timeout changes.

Currently we do it with instance_variable_get, but I'd obviously prefer not to rely on that for longer term. You could make a case that we might want a more generalized way of reconfiguring, but I think timeout is the 99% use case where you want to keep the same socket but change someting.

Thoughts?

@zanker zanker force-pushed the zanker/reset-timeouts branch from ad29f21 to c2b1e35 Compare February 7, 2016 23:00
@tarcieri
Copy link
Member

This seems like a one-off solution for the issue of timeouts when grabbing a connection from a connection pool. It'd be nice if there were a more general solution to this problem.

We can just solve cases like this as they crop up, but the intended goal of the chaining API was to be stateless. Perhaps we can abstract configuration overrides like this into a context object that can get passed in per-request?

@zanker
Copy link
Contributor Author

zanker commented Feb 10, 2016

How many options do you really need to dynamically override though? If you're assuming it's a connection pool you only really are going to toggle cookies/timeouts and that's about it.

@ixti
Copy link
Member

ixti commented Feb 10, 2016

At least these three should be configurable:

  • headers (cookies)
  • timeouts
  • redirect follow strategy (on / off / max-hops etc)

👍 on idea of context.

@zanker
Copy link
Contributor Author

zanker commented Feb 11, 2016

Hrm, maybe something like reconfigure on the client that gives you a form of chainable that modifies the @default_options? And is also filtered to headers/cookies/timeouts/redirects?

@tarcieri tarcieri mentioned this pull request Feb 16, 2016
@tarcieri
Copy link
Member

I think #306 (HTTP::Session) is the way forward for this. Closing this for now.

@tarcieri tarcieri closed this Mar 19, 2016
@ixti ixti deleted the zanker/reset-timeouts branch March 20, 2016 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants