-
Notifications
You must be signed in to change notification settings - Fork 299
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
[WIP] Timeout exception #1630
[WIP] Timeout exception #1630
Conversation
docs/responses/exceptions.md
Outdated
|
||
* `ConnectException`: when a connection (tcp socket) can't be established to the target host | ||
* `ReadException`: when a connection has been established, but there's any kind of problem receiving the response (e.g. a broken socket) | ||
* `TimeoutException`: a discriminated sub-kind of `ReadException` for timeout fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sub-type - kind
is a different thing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
@@ -228,7 +228,7 @@ class FinagleBackend(client: Option[Client] = None) extends SttpBackend[TFuture, | |||
Some(new SttpClientException.ConnectException(request, e)) | |||
case e: com.twitter.finagle.ChannelClosedException => Some(new SttpClientException.ReadException(request, e)) | |||
case e: com.twitter.finagle.IndividualRequestTimeoutException => | |||
Some(new SttpClientException.ReadException(request, e)) | |||
Some(new SttpClientException.TimeoutException(request, e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure it is the only backend that needs this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pask423 there are also individual adapters for Akka and Armeria in this PR. Other backends probably just emit standard java timeout exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then, my bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the unchanged backends emit java native exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question Can you add tests for backends?
@@ -228,7 +228,7 @@ class FinagleBackend(client: Option[Client] = None) extends SttpBackend[TFuture, | |||
Some(new SttpClientException.ConnectException(request, e)) | |||
case e: com.twitter.finagle.ChannelClosedException => Some(new SttpClientException.ReadException(request, e)) | |||
case e: com.twitter.finagle.IndividualRequestTimeoutException => | |||
Some(new SttpClientException.ReadException(request, e)) | |||
Some(new SttpClientException.TimeoutException(request, e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pask423 there are also individual adapters for Akka and Armeria in this PR. Other backends probably just emit standard java timeout exceptions.
Thanks! |
Before submitting pull request:
sbt compile
sbt compileDocs
sbt test
sbt scalafmt