+ht2 #776 handle incoming SETTINGS_HEADER_TABLE_SIZE #790

ktoso · 2017-01-18T12:03:58Z

Minimal changes to get the settings handling rolling, handle incoming SETTINGS_HEADER_TABLE_SIZE. Not handling outgoing, since we don't send them anyway.

The Rendering stage I'd like to keep, at least for now, since the Data rendering there will be nice to impl there. I'm not so very keen on making it a statefulMapConcat for now, we can later if indeed looks good?

Resolves #776

ktoso · 2017-01-18T12:04:25Z

Please review, I'll continue on top of this one with the other handlings

@akka/akka-http-team @jrudolph

akka-ci · 2017-01-18T12:11:46Z

Test PASSed.

jrudolph

Great, looking good. We don't really test so far that the hpack encoder actually uses the setting but I think that's fine for now. Maybe add a new ticket to add that test later on.

jrudolph · 2017-01-18T13:02:38Z

akka-http2-support/src/main/scala/akka/http/impl/engine/http2/FrameLogger.scala

-        case SettingsAckFrame ⇒
-          LogEntry(0, "SETA", "")
+        case SettingsAckFrame(s) ⇒
+          LogEntry(0, "SETA", s"Acked settings: ${s.mkString(",")}")


Let's use the same settingsInfo format as above.

right, done

jrudolph · 2017-01-18T13:03:34Z

akka-http2-support/src/main/scala/akka/http/impl/engine/http2/Http2Protocol.scala

+   * The size of a frame payload is limited by the maximum size that a receiver advertises in the SETTINGS_MAX_FRAME_SIZE setting.
+   * This setting can have any value between 214 (16,384) and 224-1 (16,777,215) octets, inclusive.
+   */
+  final val MinFrameSize = Math.pow(2, 14).toInt


Maybe put the integer number in the code and write the2^14 in a comment?

Also, I just discovered scaladoc understand ^12^ as ¹²

jrudolph · 2017-01-18T13:05:25Z

akka-http2-support/src/main/scala/akka/http/impl/engine/http2/framing/Http2FrameParsing.scala

@@ -100,7 +100,7 @@ private[http2] class Http2FrameParsing(shouldReadPreface: Boolean) extends ByteS
              if (payload.hasRemaining)
                throw new Http2Compliance.IllegalPayloadInSettingsAckFrame(payload.remainingSize, s"SETTINGS ACK frame MUST NOT contain payload (spec 6.5)!")

-              SettingsAckFrame
+              SettingsAckFrame(Nil)


That's the point, where we would have needed to keep track of the outgoing settings to put them in here again. Maybe add that in a comment if we decide to allow dynamic setting updates later on.

good point adding

jrudolph · 2017-01-18T13:08:32Z

akka-http2-support/src/test/scala/akka/http/impl/engine/http2/Http2ServerSpec.scala

-        pending
+        sendSETTING(SettingIdentifier.SETTINGS_HEADER_TABLE_SIZE, Math.pow(2, 15).toInt) // 32768, valid value (between 2^14 and 2^14 - 1)
+
+        expectSettingsAck()


Maybe add a TODO here that we'd like to check that the setting was actually applied (which is somewhat complicated, fill the table, then new setting, then check that evicted table entries don't end up here any more).

Added todo, though I assumed the hpack side of things is taken care of by the current impl, but yeah have not really checked this.

jrudolph · 2017-01-18T13:08:46Z

akka-http2-support/src/test/scala/akka/http/impl/engine/http2/Http2ServerSpec.scala

    def expectDecodedResponseHEADERSPairs(streamId: Int, endStream: Boolean = true): Seq[(String, String)] = {
      val headerBlockBytes = expectHeaderBlock(streamId, endStream)
      decodeHeaders(headerBlockBytes)
    }

-    val encoder = new Encoder(HeaderDecompression.maxHeaderTableSize)
+    val encoder = new Encoder(Http2Protocol.InitialMaxHeaderTableSize)


ktoso · 2017-01-18T13:37:12Z

Applied all feedback

akka-ci · 2017-01-18T13:46:45Z

Test PASSed.

jrudolph · 2017-01-18T13:50:07Z

akka-http2-support/src/main/scala/akka/http/impl/engine/http2/FrameLogger.scala

-        case SettingsAckFrame ⇒
-          LogEntry(0, "SETA", "")
+        case SettingsAckFrame(s) ⇒
+          val acksInfo = s.map {


Can you create a method to be reused with the above? In case we want to change the format later on.

jrudolph · 2017-01-18T13:51:55Z

akka-http2-support/src/main/scala/akka/http/impl/engine/http2/Http2Compliance.scala

+  def validateMaxFrameSize(value: Int): Unit = {
+    import Http2Protocol.MinFrameSize
+    import Http2Protocol.MaxFrameSize
+    if (value < MinFrameSize) throw new Http2ProtocolException(ErrorCode.FRAME_SIZE_ERROR, s"MAX_FRAME_SIZE MUST NOT be < than $MinFrameSize, attempted setting to: $value!")


6.5.2:

Values outside this range MUST be treated as a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

Right thanks for the catch, snuck in here though it was meant to be in next PR with tests I guess.

jrudolph · 2017-01-18T13:53:43Z

akka-http2-support/src/main/scala/akka/http/impl/engine/http2/Http2ServerDemux.scala

-
-            case e: Http2Compliance.IllegalHttp2FrameSize ⇒
-              pushGOAWAY(FRAME_SIZE_ERROR)
+            case e: Http2Compliance.Http2ProtocolException ⇒


akka-ci · 2017-01-18T14:11:43Z

Test PASSed.

jrudolph

LGTM

ktoso · 2017-01-18T14:24:53Z

Thanks for reviews, writing/copying the same things for the nth time caught me off guard in some places.

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Jan 18, 2017

jrudolph approved these changes Jan 18, 2017

View reviewed changes

ktoso force-pushed the wip-table-size-setting branch from 2c3cbe8 to e1772de Compare January 18, 2017 13:36

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Jan 18, 2017

jrudolph reviewed Jan 18, 2017

View reviewed changes

+ht2 akka#776 handle incoming SETTINGS_HEADER_TABLE_SIZE

2274150

ktoso force-pushed the wip-table-size-setting branch from e1772de to 2274150 Compare January 18, 2017 14:01

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Jan 18, 2017

jrudolph approved these changes Jan 18, 2017

View reviewed changes

ktoso merged commit 6c542a6 into akka:master Jan 18, 2017

ktoso deleted the wip-table-size-setting branch January 18, 2017 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

+ht2 #776 handle incoming SETTINGS_HEADER_TABLE_SIZE #790

+ht2 #776 handle incoming SETTINGS_HEADER_TABLE_SIZE #790

ktoso commented Jan 18, 2017

ktoso commented Jan 18, 2017

akka-ci commented Jan 18, 2017

jrudolph left a comment

jrudolph Jan 18, 2017

ktoso Jan 18, 2017

jrudolph Jan 18, 2017

ktoso Jan 18, 2017

ktoso Jan 18, 2017 •

edited

Loading

jrudolph Jan 18, 2017

ktoso Jan 18, 2017

jrudolph Jan 18, 2017

ktoso Jan 18, 2017

jrudolph Jan 18, 2017

ktoso commented Jan 18, 2017

akka-ci commented Jan 18, 2017

jrudolph Jan 18, 2017

ktoso Jan 18, 2017

jrudolph Jan 18, 2017

ktoso Jan 18, 2017

jrudolph Jan 18, 2017

akka-ci commented Jan 18, 2017

jrudolph left a comment

ktoso commented Jan 18, 2017

+ht2 #776 handle incoming SETTINGS_HEADER_TABLE_SIZE #790

+ht2 #776 handle incoming SETTINGS_HEADER_TABLE_SIZE #790

Conversation

ktoso commented Jan 18, 2017

ktoso commented Jan 18, 2017

akka-ci commented Jan 18, 2017

jrudolph 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

ktoso Jan 18, 2017 • edited Loading

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

ktoso commented Jan 18, 2017

akka-ci commented Jan 18, 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

akka-ci commented Jan 18, 2017

jrudolph left a comment

Choose a reason for hiding this comment

ktoso commented Jan 18, 2017

ktoso Jan 18, 2017 •

edited

Loading