Multiple streams, flyweights for PING and GOAWAY #4

jitsni · 2017-03-16T22:44:47Z

Many changes:

handling multiple streams in a connection
handling request with payload
PING frame processing
PING, GOAWAY flyweights
Using slab to deal with request fragmentation
HTTP2 stream state management (not complete)
Some bug fixes

for stream lifecycle.

* Processing PING frame * Sending GO_AWAY frame and closing connection for some validation errors * Adding code to detect slab when enough data is not available.

jfallows · 2017-03-20T20:25:58Z

pom.xml

+      <artifactId>nukleus-tcp</artifactId>
+      <version>0.3</version>
+      <scope>test</scope>
+    </dependency>


Please keep each nukleus self-contained with no inter-nukleus dependencies.

removed the dependency

jfallows · 2017-03-20T20:36:06Z

src/main/java/org/reaktivity/nukleus/http2/internal/routable/stream/Http2Stream.java

+            case CLOSED:
+                inClosed(http2RO);
+                break;
+        }


One of the design patterns we've been following is to model the states as lambdas, so the state field becomes the lambda and is called directly by the generic entry point. A state transition then updates the state field to reference the new lambda instead. Let's continue to use that approach here as well.

The state machine is not implemented completely. I will address this in future PR.

jfallows · 2017-03-20T20:37:56Z

src/main/java/org/reaktivity/nukleus/http2/internal/routable/stream/Http2Stream.java

+        if (!(http2RO.type() == Http2FrameType.HEADERS || http2RO.type() == Http2FrameType.PRIORITY))
+        {
+            connection.error(Http2ErrorCode.PROTOCOL_ERROR);
+            return;


Try to design the methods as having a single return for both increased clarity of implementation and opportunity for runtime optimization.

jfallows · 2017-03-20T20:39:44Z

src/main/java/org/reaktivity/nukleus/http2/internal/routable/stream/Http2Stream.java

+        this.state = State.IDLE;
+    }
+
+    void decode(Http2FrameFW http2RO)


We haven't been passing flyweights as method parameters, instead treating them as decoders only and keeping their scope for reuse more clear.

I would expect this method signature to follow MessageHandler.onMessage(...) signature.

Now moving into SourceInputStreamFactory, removes the need to pass flyweights

jfallows · 2017-03-20T20:43:06Z

src/main/java/org/reaktivity/nukleus/http2/internal/routable/stream/Http2Stream.java

+        CLOSED
+    }
+
+    private final SourceInputStreamFactory.SourceInputStream connection;


Passing the connection into the Http2Stream feels slightly off - perhaps it would be feasible to pass the Target for writing back to the client?

Perhaps moving this to an inner class of SourceInputStreamFactory would also help to manage the scope of the flyweights more explicitly.

I was thinking of that, but it might become a very large class

Moved Http2Stream into SourceInputStreamFactory for now

jfallows · 2017-03-20T21:21:58Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/Http2SettingsFW.java

-    public DirectBuffer payload()
-    {
-        return payloadRO;
+        });


Perhaps we are missing Stream-like utility methods on ListFW, as this inline lambda would be better specified as listFW.stream().filter(x -> x.id() == key).findFirst().
Please file an issue on maven-nukleus-plugin to request that capability on the generated ListFW flyweight.

Yes, I was thinking the same. Will file an issue.

Great, please link back to it here when filed.

Here is the issue: reaktivity/nukleus-maven-plugin#16

jfallows · 2017-03-20T21:26:26Z

src/test/java/org/reaktivity/nukleus/http2/internal/bench/Http2Server.java

+        Thread.sleep(10000000);
+    }
+
+}


This class doesn't belong in this repository.

jfallows · 2017-03-20T21:26:48Z

src/test/java/org/reaktivity/nukleus/http2/internal/bench/Http2ServerBM.java

@@ -94,7 +95,8 @@
            LangUtil.rethrowUnchecked(ex);
        }

-        reaktor = Reaktor.launch(configuration, n -> "http".equals(n), Http2Controller.class::isAssignableFrom);
+        //reaktor = Reaktor.launch(configuration, n -> "http".equals(n), Http2Controller.class::isAssignableFrom);
+        reaktor = null;


Please update to ensure this benchmark is functional.

It was just copied from Http nukleus, so it is not functional yet. It will be addressed in the future PRs

This is a useful non-functional checkpoint on the initial implementation work.

Please get the benchmark into a running state before we merge this PR, you can see a working example in the tcp nukleus.

No need to spend effort on actually optimizing the code just yet, that can be covered in future PRs no problem.

jfallows · 2017-03-20T21:28:15Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/Http2ContinuationFWTest.java

+                .header(hf -> hf.indexed(2))      // :method: GET
+                .header(hf -> hf.indexed(6))      // :scheme: http
+                .header(hf -> hf.indexed(4))      // :path: /
+                .header(hf -> hf.literal(l -> l.type(INCREMENTAL_INDEXING).name(1).value("www.example.com")))


Use h instead of hf for header, much more readable.

jfallows · 2017-03-20T21:29:23Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/Http2ContinuationFWTest.java

+        MutableDirectBuffer buf = new UnsafeBuffer(bytes);
+
+        Http2ContinuationFW fw = new Http2ContinuationFW.Builder()
+                .wrap(buf, 1, buf.capacity())


Assuming 1 here is a placeholder for any non-zero value to make sure we are calculating offsets correctly?

jfallows

Please also give feedback if removing the Http2 prefix on the flyweight class names caused any issues.

jfallows · 2017-03-22T17:15:09Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/ErrorCode.java

+            case 12 : return INADEQUATE_SECURITY;
+            case 13 : return HTTP_1_1_REQUIRED;
+        }
+        return null;


Move to default case for readability.

jfallows · 2017-03-22T17:15:57Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/FrameType.java

    {
        this.type = (byte) type;
-        Types.TYPES.put(this.type, this);
    }

    public byte getType()


This should be named type() for consistency with the others.

jfallows · 2017-03-22T17:17:24Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/FrameType.java

-    }
-
-    public static Http2FrameType from(byte type)
+    public static FrameType from(byte type)


Change to FrameType get(int type)

jfallows · 2017-03-22T17:19:13Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/GoawayFW.java

+        int streamId = Http2FrameFW.streamId(buffer, offset);
+        if (streamId != 0)
+        {
+            throw new Http2Exception(String.format("Invalid stream-id=%d for GOAWAY frame", streamId));


We don't need a custom unchecked exception type, something like IllegalArgumentException is sufficient here.

jfallows · 2017-03-22T17:20:22Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/Http2Exception.java

+        super(cause);
+    }
+}
+


Not needed, see above.

jfallows · 2017-03-22T17:25:13Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/GoAwayFWTest.java

+        byte[] bytes = new byte[100];
+        MutableDirectBuffer buf = new UnsafeBuffer(bytes);
+
+        GoawayFW fw = new GoawayFW.Builder()


Use goaway as variable name instead of fw for improved readability.

jfallows · 2017-03-22T17:25:36Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/PingFWTest.java

+        MutableDirectBuffer buf = new UnsafeBuffer(bytes);
+
+        DirectBuffer payload = new UnsafeBuffer(new byte[] {1, 2, 3, 4, 5, 6, 7, 8});
+        PingFW fw = new PingFW.Builder()


fw -> ping

jfallows · 2017-03-22T17:25:49Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/RstStreamFWTest.java

@@ -32,7 +32,7 @@ public void encode()
        byte[] bytes = new byte[100];
        MutableDirectBuffer buf = new UnsafeBuffer(bytes);

-        Http2RstStreamFW fw = new Http2RstStreamFW.Builder()
+        RstStreamFW fw = new RstStreamFW.Builder()


fw -> reset

jfallows · 2017-03-22T17:26:23Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/SettingsFWTest.java

@@ -38,7 +38,7 @@ public void decode()
        };

        DirectBuffer buffer = new UnsafeBuffer(bytes);
-        Http2SettingsFW fw = new Http2SettingsFW().wrap(buffer, 2, buffer.capacity());
+        SettingsFW fw = new SettingsFW().wrap(buffer, 2, buffer.capacity());


fw -> settings

jfallows · 2017-03-22T17:26:32Z

src/test/java/org/reaktivity/nukleus/http2/internal/types/stream/WindowUpdateFWTest.java

@@ -31,7 +31,7 @@ public void encode()
        byte[] bytes = new byte[100];
        MutableDirectBuffer buf = new UnsafeBuffer(bytes);

-        Http2WindowUpdateFW fw = new Http2WindowUpdateFW.Builder()
+        WindowUpdateFW fw = new WindowUpdateFW.Builder()


fw -> window

jfallows · 2017-03-28T00:01:44Z

src/main/java/org/reaktivity/nukleus/http2/internal/types/stream/FrameType.java

    }

-    public byte getType()
+    public byte type()


Do we need type() as an int or byte?
Doesn't seem quite right to be casting on every access when we control the storage type.

Using byte now.

conflict with core flyweights)

jitsni added 14 commits March 13, 2017 17:44

Adding a HTTP POST test case (request carries payload)

fd0ac69

Merge remote-tracking branch 'upstream/develop' into http.post.exchange

b1f6949

Creating a Http2Stream to represent HTTP2 stream which can be used

74a0590

for stream lifecycle.

Adding a simple HTTP2 server for testing

37b226e

* Not sending a frame when the received SETTINGS frame has ACK flag

ab698c2

* Processing PING frame * Sending GO_AWAY frame and closing connection for some validation errors * Adding code to detect slab when enough data is not available.

Adding a Stream state enum

6476a46

Adding a slab to deal with partial HTTP2 frames while decoding.

1081fa9

Adding a new test case where connection has two streams

a08c523

Adding flyweight for CONTINUATION frame

37c0309

Adding flyweight for RST_STREAM

629f811

SETTINGS flyweight is modified to use List flyweight

686a399

Adding flyweight for WINDOW_UPDATE frame

57b28e0

Adding slab for handling partial client preface bytes

977f80f

Updating the http2 spec version

25f7bc1

jfallows suggested changes Mar 20, 2017

View reviewed changes

jitsni added 5 commits March 21, 2017 13:05

PR feedback

5ad7eb4

Removing Http2 from names

215cf12

Using the payload(DirectBuffer) while building PING frame

e0a81a5

Refactoring idle and open states

402fd0f

Using utility method to calculate the HTTP2 frame length

afeff4a

jfallows suggested changes Mar 22, 2017

View reviewed changes

jitsni added 9 commits March 22, 2017 11:29

Minor PR feedback is incorporated

93cbfaa

Using inheritance for HTTP2 frames

10d6c9f

HTTP2 frame builders also use inheritance

1a9a513

Fixing the Http2FrameFW builder signature

b286aa3

No need to set limit() as payloadLength() sets it anyway.

93c77ee

The benchmark measures the no of HTTP2 DATA frames sent and received

26efe89

Minor formatting

af934c0

Adding PUSH_PROMISE flyweight

68e9c4c

Removing the benchmarks dir to start the benchmark in a clean state

f4a32eb

s/fw/settings

d973f20

jfallows suggested changes Mar 28, 2017

View reviewed changes

jitsni added 2 commits March 27, 2017 17:49

Adding Http2 prefix to class names for flyweights(as some of them

c477fbb

conflict with core flyweights)

Using byte instead of int for type

ab7953b

jfallows approved these changes Mar 28, 2017

View reviewed changes

jfallows merged commit 256f1f5 into reaktivity:develop Mar 28, 2017

Multiple streams, flyweights for PING and GOAWAY #4

Multiple streams, flyweights for PING and GOAWAY #4

Conversation

jitsni commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfallows left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment