Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Preserve http2 option when going from javadsl to scaladsl #2149

Merged
merged 2 commits into from
Aug 17, 2018
Merged

Preserve http2 option when going from javadsl to scaladsl #2149

merged 2 commits into from
Aug 17, 2018

Conversation

chbatey
Copy link
Member

@chbatey chbatey commented Aug 15, 2018

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
Copy link

akka-ci commented Aug 15, 2018

Test FAILed.

@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
Copy link

akka-ci commented Aug 16, 2018

Test PASSed.

@johanandren
Copy link
Member

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
Copy link
Member

@raboof raboof left a comment

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

akka-http-core/src/main/scala/akka/http/javadsl/ConnectHttp.scala Outdated
@@ -167,6 +168,9 @@ final class ConnectHttpImpl(val host: String, val port: Int, val http2: UseHttp2
def isHttps: Boolean = false

def connectionContext: Optional[HttpsConnectionContext] = Optional.empty()

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

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..

Copy link
Member

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.

Copy link
Member 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.

Copy link
Member 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

Copy link
Member

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
Copy link
Member

@raboof raboof left a comment

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
Copy link
Member

@jrudolph jrudolph left a comment

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
Copy link
Member Author

chbatey commented Aug 16, 2018

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

chbatey
chbatey commented Aug 16, 2018
View reviewed changes
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?
Copy link
Member 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
Copy link
Member

@raboof raboof left a comment

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
Copy link

akka-ci commented Aug 16, 2018

Test FAILed.

@jrudolph
Copy link
Member

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
Copy link

akka-ci commented Aug 16, 2018

Test FAILed.

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

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
Copy link

akka-ci commented Aug 16, 2018

Test PASSed.

@jrudolph
Copy link
Member

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
Copy link

akka-ci commented Aug 16, 2018

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
Copy link

akka-ci commented Aug 16, 2018

Test PASSed.

chbatey
chbatey commented Aug 16, 2018
View reviewed changes
Copy link
Member Author

@chbatey chbatey left a comment

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

@jrudolph jrudolph jrudolph left review comments

@raboof raboof raboof approved these changes

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