finagle-http: Implement streaming disconnection tests #487
Conversation
|
cc @mosesn |
olix0r
force-pushed
the
ver/test-streaming-disconnects
branch
from
1b8d78d
to
cf4c8c5
Compare
|
Does this need the two patches I added here: https://github.com/mosesn/finagle/commits/mnakamura/test-streaming-disconnects ? |
|
@mosesn indeed it does... |
olix0r
force-pushed
the
ver/test-streaming-disconnects
branch
2 times, most recently
from
ddd5f64
to
090396e
Compare
Problem finagle-http's streaming test coverage misses some important cases Solution Introduce new tests to http.StreamingTest to validate that 1. A client behaves well when a connection is lost before a server responds. 2. A server closes the transport after responding, as described in twitter#475.
olix0r
force-pushed
the
ver/test-streaming-disconnects
branch
from
090396e
to
d0938ab
Compare
|
This branch looks right to me, but it fails (which either means there's still a bug or the tests are bad...) These failures originate in assertSecondRequestOk. |
|
61f193a appears to break these tests. Specifically, the following: def connect(addr: SocketAddress, mod: Modifier, name: String = "client") = {
val fac = ClientBuilder()
.codec(new Custom(mod, identity))
.hosts(Seq(addr.asInstanceOf[InetSocketAddress]))
.hostConnectionLimit(1)
.name(name)
.buildFactory()
await(fac()
}This obtains a single connection such that when the transport fails, subsequent requests fail. Tests pass when this is changed to: def connect(addr: SocketAddress, mod: Modifier, name: String = "client") =
ClientBuilder()
.codec(new Custom(mod, identity))
.hosts(Seq(addr.asInstanceOf[InetSocketAddress]))
.hostConnectionLimit(1)
.name(name)
.build() |
|
@olix0r that patch seems to be nonsense. I don't understand it, or why either of us thought it was a good idea (you seem to have merged it in here: https://github.com/BuoyantIO/finagle/pull/3/files. Trying to find the original on my hard drive in case github did something funny here. |
|
Nope, github is correct, that commit is just a bunch of nonsense. I'll try to figure out what I was thinking about again. |
|
OK I just have no idea what that commit was supposed to do, but I think the test might not be right now that I'm thinking about it. If the client succeeds in writing all of its bytes to the wire, it won't realize that the remote has closed the connection until it next tries to write over the connection, so that test is inherently racy. |
|
Yep, the client: server disconnect on pending response should fail request test times out because the response is never triggered. I could change this to attempt a write after its closed, but maybe that test isn't particularly useful. |
|
The good news is that I figured out what that patch was supposed to do! The |
|
@olix0r it seems like adding a promise that you satisfy when the request has been received, and awaiting that promise also fixes the test. |
|
OK, now that I think about it more, I'm confused. Why does this fix it? Why would the server never receiving the request mean that we should time out? |
|
@mosesn is it because the I've changed it as follows and the test now passes: test("client: server disconnect on pending response should fail request") {
val failure = new Promise[Unit]
val service = Service.mk[Request, Response] { req =>
failure.setDone()
Future.never
}
val server = startServer(service, closingTransport(failure))
val client = connect(server.boundAddress, identity)
val resF = client(get("/"))
intercept[ChannelClosedException] { await(resF) }
await(client.close())
await(server.close())
} |
|
@mosesn but now i'm confused -- how is this at all functionally different than: test("client: server disconnect on pending response should fail request") {
val failure = new Promise[Unit]
val service = Service.mk[Request, Response] { req =>
Future.never
}
val server = startServer(service, closingTransport(failure))
val client = connect(server.boundAddress, identity)
val resF = client(get("/"))
failure.setDone()
intercept[ChannelClosedException] { await(resF) }
await(client.close())
await(server.close())
}I assume that failure is being satisfied before the server processes the request -- why does this timeout but the other mode, where the server fully receives the request, fails? |
|
OK, so thinking about it more (and adding a bunch of printlns), I think that the problem is that the client says it has successfully established a connection when the kernel finishes the handshake, and the server might not have called accept and assigned a transport yet. So there's a race between The smallest diff I've found that can fix the test is removing the |
A failure could be satisfied before the transport was provisioned, causing the transport not to be closed. Provide reusable transport-closing helper.
|
Thaaaaaaanks @olix0r |
|
Made it to |
Problem
finagle-http's streaming test coverage misses some important cases
Solution
Introduce new tests to http.StreamingTest to validate that