Preserve http2 option when going from javadsl to scaladsl by chbatey · Pull Request #2149 · akka/akka-http · GitHub
Skip to content

Preserve http2 option when going from javadsl to scaladsl - #2149

Merged
raboof merged 2 commits into
akka:masterfrom
chbatey:javadsl-http2-no-tls
Aug 17, 2018
Merged

Preserve http2 option when going from javadsl to scaladsl#2149
raboof merged 2 commits into
akka:masterfrom
chbatey:javadsl-http2-no-tls

Conversation

@chbatey

@chbatey chbatey commented Aug 15, 2018

Copy link
Copy Markdown
Contributor

The only information passed from javadsl to the scaladsl in Http
was the context which for Http connections was always the default one
meaning that setting http2 always had no effect.

This seed the easiest way not to break the user facing API

@chbatey
chbatey requested a review from raboof August 15, 2018 15:53
@chbatey chbatey mentioned this pull request Aug 15, 2018
Merged
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Aug 15, 2018
@akka-ci

akka-ci commented Aug 15, 2018

Copy link
Copy Markdown

@chbatey
chbatey force-pushed the javadsl-http2-no-tls branch from 0c6a4ec to 7c23395 Compare August 16, 2018 06:26
@chbatey chbatey mentioned this pull request Aug 16, 2018
Merged
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Aug 16, 2018
@akka-ci

akka-ci commented Aug 16, 2018

Copy link
Copy Markdown

Test PASSed.

@johanandren

johanandren commented Aug 16, 2018

Copy link
Copy Markdown
Contributor

I think this refs #2110 ?

@johanandren johanandren mentioned this pull request Aug 16, 2018
Closed
@chbatey chbatey changed the title Preserve http2 option when doing from javadsl to scaladsl Preserve http2 option when going from javadsl to scaladsl Aug 16, 2018
raboof
raboof reviewed Aug 16, 2018
View reviewed changes

@raboof raboof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty tricky to understand this structure, digging into it a bit more

Comment thread akka-http-core/src/main/scala/akka/http/javadsl/ConnectHttp.scala Outdated
def connectionContext: Optional[HttpsConnectionContext] = Optional.empty()

override def effectiveConnectionContext(fallbackContext: ConnectionContext): ConnectionContext =
HttpConnectionContext(http2.asScala)

@raboof raboof Aug 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this always ignores the host, port, connectionContext and the fallbackContext parameter? That seems odd..

@raboof raboof Aug 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah connectionContext is the HTTPS context so that might make sense to ignore here.

@chbatey chbatey Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♠️

This did take me a couple of hours 😕

I find the API very confusing bewtween the ConnectHttp and Context.

the connectionContext is only Https so a user can't ever have called withContext and set a custom one here.

I think that is a restriction in the API that we could remove but it wouldn't be binary/source compatible so this was a quick way to make it work without breaking any existing users.

@chbatey chbatey Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host and port are ignored here but when the delegation to the scaladsl from the javadsl they are taken from the ConnectHttp and passed in

@raboof raboof Aug 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah seems to make sense, just trying to digest the change ;)

raboof
raboof approved these changes Aug 16, 2018
View reviewed changes

@raboof raboof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so if I'm reading this right, what makes this confusing is that the http2 field is present both on the ConnectHttp and on the ConnectionContext. This is different from the host and port fields, which are only on the ConnectHttp. For HTTP, ConnectionContext actually doesn't contain much interesting information, while for HTTPS it does have some settings.

I agree for HTTP, just creating a HttpConnectionContext with the http2 setting seems fine: the ConnectionContext doesn't contain other interesting configuration so ignoring the 'fallback' is harmless.

For HTTPS it's a bit more complicated, but seems OK as well since if you're overriding HttpsConnectionContext you should be knowing what you're doing anyway.

Would appreciate a second review though :).

jrudolph
jrudolph reviewed Aug 16, 2018
View reviewed changes

@jrudolph jrudolph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see with this solution is that 1. it might break down if we add other fields to HttpConnectionContext in the future and 2. that the flag seems still to be lost for the HTTPS case.

I tried a slightly different approach here: chbatey#1

@chbatey

chbatey commented Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

I prefer @jrudolph's solution. Do you want to take another look @raboof ?

chbatey
chbatey commented Aug 16, 2018
View reviewed changes
Comment thread akka-http-core/src/main/scala/akka/http/javadsl/ConnectHttp.scala Outdated
if (connectionContext.isPresent) connectionContext.get()
else fallbackContext
connectionContext
.map[ConnectionContext](c ⇒ c: ConnectionContext) // covariance, anyone?

@chbatey chbatey Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed :(

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Aug 16, 2018
raboof
raboof approved these changes Aug 16, 2018
View reviewed changes

@raboof raboof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, seems sensible!

@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Aug 16, 2018
@akka-ci

akka-ci commented Aug 16, 2018

Copy link
Copy Markdown

Test FAILed.

@jrudolph

jrudolph commented Aug 16, 2018

Copy link
Copy Markdown
Contributor

PLS BUILD

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Aug 16, 2018
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Aug 16, 2018
@akka-ci

akka-ci commented Aug 16, 2018

Copy link
Copy Markdown

Test FAILed.

@jrudolph
jrudolph force-pushed the javadsl-http2-no-tls branch from 4f5d006 to f1f47f4 Compare August 16, 2018 13:53
@jrudolph

jrudolph commented Aug 16, 2018

Copy link
Copy Markdown
Contributor

Compilation failed with a Scala 2.11 type inference problem... Hopefully fixed now :)

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Aug 16, 2018
@akka-ci

akka-ci commented Aug 16, 2018

Copy link
Copy Markdown

Test PASSed.

@jrudolph

jrudolph commented Aug 16, 2018

Copy link
Copy Markdown
Contributor

Hey, scalac 2.11, you are a pain (though, I know having to deal with Java existentials is not your fault). Great, however, that these bugs have been fixed in 2.12!

@jrudolph
jrudolph force-pushed the javadsl-http2-no-tls branch from f1f47f4 to 69df59e Compare August 16, 2018 14:53
chbatey and others added 2 commits August 16, 2018 16:54
@chbatey @jrudolph
Preserve http2 option when doing from javadsl to scaladsl
6b5b7ed 
The only information passed from javadsl to the scaladsl in Http
was the context which for Http connections was always the default one
meaning that setting http2 always had no effect.
@jrudolph
=htp Make sure http2 flag is applied to effective context in all cases
afe0f24
@jrudolph
jrudolph force-pushed the javadsl-http2-no-tls branch from 69df59e to afe0f24 Compare August 16, 2018 14:55
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Aug 16, 2018
@akka-ci

akka-ci commented Aug 16, 2018

Copy link
Copy Markdown

Test FAILed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Aug 16, 2018
@akka-ci

akka-ci commented Aug 16, 2018

Copy link
Copy Markdown

chbatey
chbatey commented Aug 16, 2018
View reviewed changes

@chbatey chbatey left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for finishing this!

@raboof
raboof merged commit 12ae897 into akka:master Aug 17, 2018
@raboof raboof added this to the 10.1.4 milestone Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@jrudolph jrudolph jrudolph left review comments
@raboof raboof raboof approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tested PR that was successfully built and tested by Jenkins

Projects

None yet

Milestone

10.1.4

Development

Successfully merging this pull request may close these issues.

5 participants

@chbatey @akka-ci @johanandren @jrudolph @raboof