[#84] Fix issue where HttpDechunker could send EOF while other chunks have not yet been read #85

Closed
wants to merge 8 commits into
from

Projects

None yet

2 participants

@stephenjudkins

Fix for issue #84

stephenjudkins added some commits May 10, 2012
@stephenjudkins stephenjudkins [#84] Fix issue where HttpDechunker could send EOF while other chunks…
… have not yet been read.
1d98f36
@stephenjudkins stephenjudkins Fix typo 175de17
@stephenjudkins stephenjudkins [#84] Added a test and fixed other breaking tests.
This is difficult to test because it relies on "setReadable" not propogating fast enough. Messages needs to be sent to HttpDechunker after setReadable(false), which does happen.
6820bb4
@mariusae
Member

I think doing something like this,

diff --git a/finagle/finagle-stream/src/main/scala/com/twitter/finagle/stream/HttpDechunker.scala b/finagle/finagle-stream/src/main/scala/com/twitter/finagle/stream/HttpDechunker.scala
index 52faaa6..fb7c7e9 100644
--- a/finagle/finagle-stream/src/main/scala/com/twitter/finagle/stream/HttpDechunker.scala
+++ b/finagle/finagle-stream/src/main/scala/com/twitter/finagle/stream/HttpDechunker.scala
@@ -40,11 +40,13 @@ class HttpDechunker extends BrokerChannelHandler {

           if (chunk.isLast) {
             ch.close()
-            sendOf andThen error(err, EOF)
+            sendOf.sync() ensure error(err, EOF)
           } else {
             ch.setReadable(false)
-            sendOf andThen ch.setReadable(true)
-            read(ch, out, err, close)
+            sendOf.sync() ensure {
+              ch.setReadable(true)
+              read(ch, out, err, close)
+            }
           }

is a bit simpler, doesn't involve the use of a Future, and makes the sequencing more explicit.

stephenjudkins added some commits May 15, 2012
@stephenjudkins stephenjudkins Changed to simpler fix, per marius a. eriksen's suggestion.
Instead of passing along a future representing the last successful write to the recursive `read` method, we instead wait to recursively call `read` until after the last read on `messages` is complete, thus avoiding the `error`/`messages` race. This way we don't have to pass along an extra Future.
a962e46
@stephenjudkins stephenjudkins Clean up test fa23333
@stephenjudkins

That is a much simpler solution to the main problem. I've updated the pull request to use it.

@mariusae mariusae and 1 other commented on an outdated diff May 16, 2012
...t/scala/com/twitter/finagle/stream/EndToEndSpec.scala
@@ -239,6 +239,22 @@ object EndToEndSpec extends Specification {
latch.within(1.second)
result mustEqual "1223"
}
+
+ "sends EOF only after buffered messages are received" in {
+ val clientRes = client(httpRequest)(1.second)
+
+ FuturePool.defaultPool {
+ messages !! ChannelBuffers.wrappedBuffer("1".getBytes)
+ messages !! ChannelBuffers.wrappedBuffer("2".getBytes)
+ error !! EOF
+ }
+
+ val firstMessage = clientRes.messages.sync()(1.second)
+
+ firstMessage must_== ChannelBuffers.wrappedBuffer("1".getBytes)
+ clientRes.error.sync()(1.second) must throwA[TimeoutException]
@mariusae
mariusae May 16, 2012 Twitter, Inc. member

I'd like to avoid having timeouts like this in the test. It is not very reliable. (The rest of specs in this file have pretty bad style wrt this, too). Perhaps you can test receive ordering explicitly?

val of = Offer.choose(clientRes.messages map(Some(_)), clientRes.errors map { _ => None })
of.syncWait() must beSome("1".getBytes)
of.syncWait() must beSome("2".getBytes)
of.syncWait() must beNone
@stephenjudkins
stephenjudkins May 16, 2012

I don't like it either, but the above solution doesn't test the bug that was fixed. If I revert the changes in HttpDechunker, the current test fails. However, your suggested test passes in either case.

The issue isn't that the EOF is sent to error before the last message is sent to messages, it's that it's sent before the last message is received and synced from messages. Thus, selecting between messages and error will most often select messages first. What we want to ensure is that EOF is not sent to error until the last message in messages has been synced.

We can verify whether the dechunker has sent the second message to messages, but we can't verify whether it's quiescent, or getting ready to immediately send EOF to error. The 1-second wait verifies to a high degree of certainty (absent a huge GC pause) that the dechunker is, in fact, waiting to send EOF until after the last message is synced.

If you're opposed to this solution, the only real alternative I can think of is unit-testing HttpDechunker without IO or an executor. If we remove threads from the equation, we can be sure that all side-effects from sending a message have occurred when we send the original message. Would you like me to write this test? Or do you have any other suggestions?

@stephenjudkins

See above commit for deterministic test. Is this a reasonable approach?

@mariusae

Can you use the JUnit style unittests? Internally we use maven+surefire to test this stuff.

@mariusae
Member

Very nice. Just the one nit.

@stephenjudkins

I don't know what you mean by JUnit style unittests. Can you point me to an example in Finagle I should emulate? Should I just switch superclass to org.specs.SpecificationWithJUnit?

Also, should I remove the additional case in EndToEndSpec?

@mariusae
Member

Pretty much any Spec, eg.: https://github.com/twitter/finagle/blob/master/finagle-native/src/test/scala/com/twitter/finagle/ssl/SslSpec.scala

Note that it is a class (not an object) and mixes in SpecificationWithJUnit not Specification

@mariusae
Member

And yes, please remove the additional case in EndToEnd. Thanks!

@stephenjudkins

I've made those changes, and merged in master. I'm assuming it's ready for merge?

I've also rebased these changes to stephenjudkins@d62455e. Pick your poison on which one you'd like.

@mariusae
Member

Looks great! I'm going to cherry pick in the other commit.

@mariusae
Member

I had to add the following build dep. Did you forget to add it?

diff --git a/project/Build.scala b/project/Build.scala
index d7923d4..752f011 100644
--- a/project/Build.scala
+++ b/project/Build.scala
@@ -206,7 +206,7 @@ object Finagle extends Build {
       sharedSettings
   ).settings(
     name := "finagle-stream"
-  ).dependsOn(finagleCore, finagleKestrel)
+  ).dependsOn(finagleCore, finagleKestrel, finagleTest % "test")

   lazy val finagleThrift = Project(
     id = "finagle-thrift",
@stephenjudkins

Yes, it looks like I forgot to add it. Sorry about that one. I was switching between SBT 0.7 and 0.11 branches so it got lost in the confusion.

@mariusae
Member

Pushed: 919fd54

@mariusae mariusae closed this May 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment