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

Connecting timeout #2736

Merged
merged 7 commits into from Jul 30, 2019

Conversation

@RafalSumislawski
Copy link
Contributor

commented Jul 21, 2019

Follow-up to http4s/blaze#308.

WIP: It won't compile until the aforementioned PR is merged and blaze 0.14.7 is released.

This PR:

  • gives users ability to provide custom TickWheelExecutor in place of the one allocated by BlazeClientBuilder
  • adds connectingTimeout for configuring the newly introduced connectingTimeout in blaze's ClientChannelFactory

Fixes: #2386, #2338, #2690

@rossabaker
Copy link
Member

left a comment

This looks good. A couple ideas below. And thanks for targeting series/0.20 with this. I'd like to get this into a patch release of http4s as soon as possible.

@@ -20,6 +23,7 @@ sealed abstract class BlazeClientBuilder[F[_]] private (
val responseHeaderTimeout: Duration,
val idleTimeout: Duration,
val requestTimeout: Duration,
val connectingTimeout: Duration,

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jul 22, 2019

Member

Bikeshed: async-http-client and jetty-client both call this connectTimeout. Maybe that's a better name for consistency. Would also affect the blaze PR.

@@ -33,6 +37,7 @@ sealed abstract class BlazeClientBuilder[F[_]] private (
val parserMode: ParserMode,
val bufferSize: Int,
val executionContext: ExecutionContext,
val scheduler: Option[TickWheelExecutor],

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jul 22, 2019

Member

I think I like the Ember idiom for defaultable resources, where the builder provides a default, or an unmanaged one can be passed in.

RafalSumislawski added some commits Jul 25, 2019

@RafalSumislawski RafalSumislawski changed the title WIP: Connecting timeout Connecting timeout Jul 29, 2019

@RafalSumislawski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

The build fails due to binary compatibility issue in BlazeClientBuilder and Http1Supoport constructors. Both constructors are private. Do we really care about binary compatibility of private APIs? @rossabaker how should I approach this topic?

@rossabaker

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

It should be safe to add a MiMa exclusion to the build for both of those.

@RafalSumislawski RafalSumislawski force-pushed the RafalSumislawski:connecting-timeout branch from 62d33d1 to 95f08e5 Jul 29, 2019

@rossabaker

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I think both test failures are flakes. Restarted.

@rossabaker
Copy link
Member

left a comment

👍 Thank you!

@rossabaker rossabaker merged commit d7d275d into http4s:series/0.20 Jul 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.