New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collision-resistent maps in header parsing #2276

Merged
merged 3 commits into from Nov 20, 2018

Conversation

Projects
None yet
5 participants
@raboof
Copy link
Member

raboof commented Nov 8, 2018

No description provided.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Nov 8, 2018

Test FAILed.

Fix (brittle, overly strict) test assumptions
AFAICS these don't point to us breaking any particular
promises, just 'different' behavior.
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Nov 8, 2018

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Nov 8, 2018

Test PASSed.

@johanandren
Copy link
Member

johanandren left a comment

LGTM

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Nov 9, 2018

Added parameters to the Content-Type in HeaderParserBenchmark, seems OK:

Master:

Cnt     Score     Error   Units
[info] HeaderParserBenchmark.bench_parse_headers                      no  thrpt    3  3598.170 ± 375.241  ops/ms
[info] HeaderParserBenchmark.bench_parse_headers                     yes  thrpt    3  3480.690 ± 209.644  ops/ms

Cnt     Score     Error   Units
[info] HeaderParserBenchmark.bench_parse_headers                      no  thrpt    3  3529.619 ± 729.152  ops/ms
[info] HeaderParserBenchmark.bench_parse_headers                     yes  thrpt    3  3565.951 ± 158.762  ops/ms


Branch:

Cnt     Score     Error   Units
[info] HeaderParserBenchmark.bench_parse_headers                      no  thrpt    3  3573.066 ±  64.581  ops/ms
[info] HeaderParserBenchmark.bench_parse_headers                     yes  thrpt    3  3643.082 ± 142.643  ops/ms

Cnt     Score      Error   Units
[info] HeaderParserBenchmark.bench_parse_headers                      no  thrpt    3  3502.156 ± 1586.796  ops/ms
[info] HeaderParserBenchmark.bench_parse_headers                     yes  thrpt    3  3608.040 ±  167.558  ops/ms
Authorization(GenericHttpCredentials("Fancy", Map("yes" → "no", "nonce" → """4\2""")))
"""Authorization: Other yes=no,empty=""""" =!=
Authorization(GenericHttpCredentials("Other", Map("yes" → "no", "empty" → "")))
Authorization(GenericHttpCredentials("Fancy", Map("nonce" → "42", "yes" → "n:o"))).renderedTo(

This comment has been minimized.

@jlprat

jlprat Nov 19, 2018

Member

I understand this change is because now using TreeMap the contents are sorted by key, right?

This comment has been minimized.

@raboof

raboof Nov 20, 2018

Member

Correct. I don't think we ever promised to keep the order of the keys (and still don't), so in that sense this test is too specific.

@jrudolph
Copy link
Member

jrudolph left a comment

LGTM. Changing the order might fail a few tests in user code but I don't think it matters because the ordering was arbitrary before anyway.

@@ -62,7 +64,7 @@ private[parser] trait LinkHeader { this: Parser with CommonRules with CommonActi
}
}

def `link-media-type` = rule { `media-type` ~> ((mt, st, pm) getMediaType(mt, st, pm contains "charset", pm.toMap)) }
def `link-media-type` = rule { `media-type` ~> ((mt, st, pm) getMediaType(mt, st, pm contains "charset", TreeMap(pm: _*))) }

This comment has been minimized.

@jrudolph

jrudolph Nov 20, 2018

Member

This constructor is probably not super fast but it probably doesn't matter for that particular header.

@jrudolph jrudolph added this to the 10.1.6 milestone Nov 20, 2018

@jrudolph jrudolph merged commit 6b224f1 into master Nov 20, 2018

4 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph deleted the collisionResistentMapsParsingHeaders branch Nov 20, 2018

jrudolph added a commit to jrudolph/akka-http that referenced this pull request Dec 20, 2018

Collision-resistent maps in header parsing (akka#2276)
(cherry picked from commit 6b224f1)

# Conflicts:
#	akka-http-core/src/test/scala/akka/http/impl/engine/parsing/HttpHeaderParserSpec.scala

jrudolph added a commit to jrudolph/akka-http that referenced this pull request Dec 20, 2018

raboof added a commit that referenced this pull request Dec 21, 2018

[10.0 backport] Collision-resistant hashMap fixes (#2343)
* [backport] Use collision-resistent maps for formFieldMaps (#2274)

(cherry picked from commit 33871be)

* [backport] collision-resistent maps in header parsing (#2276)

(cherry picked from commit 6b224f1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment