search

jetty-project / jetty-alpn

Implementation of ALPN (Application Layer Protocol Negotiation) Specification for OpenJDK 7 or greater
48 stars 27 forks source link

selected() not run for cached sessions #5

Closed ejona86 closed 9 years ago

ejona86 commented 10 years ago

If a session is resumed, supports() and protocols() are called, but selected() is never called. It seems that for cached sessions severHello() returns at ClientHandshaker.java:631, whereas ALPN selected() processing starts on line 662.

sbordet commented 10 years ago

@ejona86 thanks for reporting this. I will look at it next week; if you have a test case that reproduces the issue, that will be great.

ejona86 commented 10 years ago

I'll try to cook one up.

In short, it would just create two connections to the same host. I needed a sleep between creating the two connections to guarantee breakage, but otherwise it was straight-forward.

ejona86 commented 9 years ago

Failing test case: 0a08b3888996a8555d075e7c49386133eb92f3a6 . Things to note:

  1. The test fails on the second clientConnectSuccessful(), waiting for the ALPN latch to complete, but the SSL handshake has already completed
  2. If you change the latch on line 270 to 1 instead of 2, the test completes. protocols() is being called, but not selected()
  3. If you change "localhost" to "127.0.0.1" on line 270, the second clientConnectSuccessful() completes, but session reuse didn't occur so the assertSame fails
sbordet commented 9 years ago

@ejona86, thanks for your test case. I ended up rewriting from scratch all the test cases so I could not merge your contribution, but your test case was important to identify clearly the issue and fixing it.

Scottmitch commented 9 years ago

Great work all!

@sbordet - Is 8.1.2.v20141202 on its way to maven central?

Also will http://www.eclipse.org/jetty/documentation/current/alpn-chapter.html be updated to reflect the new version usage? For example does this unify all versions for java8 and java7...or will the older versions (pre java 1_8_25, 1_7_71) still have this issue?

sbordet commented 9 years ago

@Scottmitch it's already there.

Scottmitch commented 9 years ago

Perhaps it is still in transit? I'm still not seeing it: http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22org.mortbay.jetty.alpn%22%20AND%20a%3A%22alpn-boot%22

I also don't see the eclipse website updated with the new version numbers?

Scottmitch commented 9 years ago

I'm mostly curious if this change was made to the 7.1.0 and 8.1.0 versions (to have support for older jdk versions) and then forward ported to make 7.1.2 and 8.1.2. It doesn't look like this is the case?

joakime commented 9 years ago

@Scottmitch the content on search.maven.org lags a bit. (its updated as of a few minutes ago). You can always skip the search database and look at the location on-disk (so to speak) and see if its there ...

eg: http://central.maven.org/maven2/org/mortbay/jetty/alpn/alpn-boot/

sbordet commented 9 years ago

@Scottmitch The change has been applied to 7.1.1 for 7u72 and 8.1.1 for 8u25. No backports planned.

Scottmitch commented 9 years ago

@joakime - Thanks for the heads up.

@sbordet - Thanks for the confirmation about no backports. Just to confirm this issue does apply to the "older" versions of alpn-boot? Was there a reason why the change was not applied to the "old" version and forward ported? Seems like it would keep things consistent and make future changes easier if other issues are discovered (and also provide more correctness for these older java versions).

sbordet commented 9 years ago

@Scottmitch the issue applies to all versions apart those fixed. The fix has been applied only to the latest ALPN version, which applies to N latest OpenJDK version, in this case 7u71, 7u72 and 8u25. Older versions of ALPN match older OpenJDK versions that have been obsoleted.

Scottmitch commented 9 years ago

@sbordet - I think the scheme of coupling the alpn-boot version to OpenJDK works well to differentiate when upstream JDK releases have been merged. However I think it is a bit misleading if there is an alpn-boot specific bug that spans multiple versions as to which are safe/supported and which are not. Are there any limitations with the older alpn version such they depend upon the new OpenJDK features in order to get this patch? If the plan is to ignore the older alpn-boot versions (which I'm not sure I fully understand in this instance) then do we have, or is there a plan to have either of the following:

  1. Change log for new versions (which indicate which fixes were patched...so defects can be inferred for older versions).
  2. Advertise known defects/limitations associated with each alpn-boot version that are listed as "supported" on the eclipse website.
Scottmitch commented 9 years ago

@sbordet - The backporting has been done and submitted in PRs https://github.com/jetty-project/jetty-alpn/pull/7 (java7) and https://github.com/jetty-project/jetty-alpn/pull/6 (java8). All that needs to be done is to issue a new version number and maintain a branch for the older JDK code so alpn-boot bugs can be fixed.