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.

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

selected() not run for cached sessions #5

Closed
ejona86 opened this issue Nov 20, 2014 · 14 comments
Closed

selected() not run for cached sessions #5

ejona86 opened this issue Nov 20, 2014 · 14 comments

Comments

@ejona86
Copy link

ejona86 commented Nov 20, 2014

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

sbordet commented Nov 21, 2014

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

ejona86 commented Nov 21, 2014

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

ejona86 commented Nov 23, 2014

Failing test case: 0a08b38 . 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 added a commit that referenced this issue Dec 2, 2014
Now properly calling the selected() method on the client in case of
SSL session resumption.

Also completely rewrote tests to cover more cases.
@sbordet sbordet closed this as completed in d6eefa7 Dec 2, 2014
@sbordet
Copy link
Member

sbordet commented Dec 2, 2014

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

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

sbordet commented Dec 2, 2014

@Scottmitch it's already there.

@Scottmitch
Copy link
Contributor

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

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

joakime commented Dec 2, 2014

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

sbordet commented Dec 2, 2014

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

@Scottmitch
Copy link
Contributor

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

sbordet commented Dec 2, 2014

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

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

@sbordet - The backporting has been done and submitted in PRs #7 (java7) and #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants