Match on ClosedChannelException for EmptyStream since it's being adap… #5247
Conversation
|
I've been chasing a lot of things. Which release did you identify the bug on? Are you running 1.0 or something earlier? |
|
I ask because we might want to cherry-pick this back to another branch. I'm expecting to run another release train this week. I don't want you or anyone else to waste time on that merge conflict if this is getting backported. |
|
Looks like at least 0.23. |
|
The bugfix Chris made (#5064) was merged into |
| .toOption | ||
| .get | ||
| val request = GET(body, uri) | ||
| EmberClientBuilder.default[IO].withIdleConnectionTime(1.seconds).build.use { client => |
There was a problem hiding this comment.
I believe this is the opposite of what we want. we are interested in seeing what happens when the client timeout is longer than the server timeout i.e. what happens if the server closes their end of the connection and the client try to make another request
There was a problem hiding this comment.
Also the right client configuration option to use here is withIdleTimeInPool -- idle connection time controls how much time the client will wait for a response after a request has been sent before timeout
There was a problem hiding this comment.
Ugh, actually, I don't think this will test exactly what we want. At worst this will induce a connection reset by peer error which isn't what we want. I think we should just merge in the fix for now and think about how to do the test later, since none of the servers we have available let us emulate that behavior
There was a problem hiding this comment.
@rossabaker Updated the PR.
matthughes
force-pushed
the
emptyStream
branch
from
3bc7ea4
to
10fa477
Compare
|
I removed the test and targeted it to 0.21 branch. This should be ok to be cherry picked forward. |
| @@ -223,6 +223,7 @@ private[client] object ClientHelpers { | |||
| // case Left(EmberException.EmptyStream()) => true // Next version can be accessed by users | |||
| case Left(org.http4s.ember.core.EmptyStreamError()) => | |||
There was a problem hiding this comment.
i'm guessing this match clause isn't necessary anymore but deferring to @ChristopherDavenport on that one
There was a problem hiding this comment.
Yeah I wasn't sure as to whether we should keep that in there or not. It's not currently getting matched on.
There was a problem hiding this comment.
It looks to me like it can only be raised where it has been adapted. I think it can go.
…ted above.