eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.32k stars 2.08k forks source link

Save parsing twice numeric IPv4 address #5307

Closed franz1981 closed 2 months ago

franz1981 commented 2 months ago

Using machines which provide Host headers in the form "y.x.z.w:port" (ith y, x, z, w numeric) will cause HostAndPortImpl ::parseHost to execute an additional parseRegName because parseIPv4Address returns -1, failing to detect the address as a valid numeric address with a maybe valid port.

franz1981 commented 2 months ago

PTAL @vietj I'm still adding few others improvement and a benchmark to further improve this, but I need first confirmation of my RFC interpretation

franz1981 commented 2 months ago

These are the numbers before

Benchmark                                        (host)  Mode  Cnt   Score   Error  Units
HostAndPortBenchmark.parseAuthority    192.168.0.1:8080  avgt   20  26.070 ± 1.790  ns/op
HostAndPortBenchmark.parseHost         192.168.0.1:8080  avgt   20  16.042 ± 0.030  ns/op
HostAndPortBenchmark.parseIPv4Address  192.168.0.1:8080  avgt   20  11.542 ± 0.023  ns/op

and this PR:

Benchmark                                        (host)  Mode  Cnt   Score   Error  Units
HostAndPortBenchmark.parseAuthority    192.168.0.1:8080  avgt   20  19.384 ± 1.116  ns/op
HostAndPortBenchmark.parseHost         192.168.0.1:8080  avgt   20  10.988 ± 0.227  ns/op
HostAndPortBenchmark.parseIPv4Address  192.168.0.1:8080  avgt   20  10.627 ± 0.052  ns/op

and the additional one:

Benchmark                                        (host)  Mode  Cnt   Score   Error  Units
HostAndPortBenchmark.isValidAuthority  192.168.0.1:8080  avgt   20  13.785 ± 0.016  ns/op

The new isValidAuthority method "could" be used in vertx-web at https://github.com/vert-x3/vertx-web/blob/84dfc9a78b246089bf5a8e7282ed119c05e0efa6/vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java#L77 to save allocating HostAndPortImpl given that we just check that's a valid host, wdyt?