8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl#808
8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl#808jerboaa wants to merge 2 commits into
Conversation
|
👋 Welcome back sgehwolf! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
Bot
changed the title
|
This backport pull request has now been updated with issue from the original commit. |
openjdk
Bot
added
backport
|
Adding a dependency on #804 so that a backport of JDK-8284047 can depend on this one with all the dependencies in place. |
|
|
openjdk-notifier
Bot
force-pushed
the
pr/804
branch
from
e415543 to
2314f6b
Compare
jerboaa
force-pushed
the
8259662-dont-wrap-socketexcpn-8u
branch
from
3c06053 to
0dc5b58
Compare
|
@jerboaa Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
openjdk-notifier
Bot
force-pushed
the
pr/804
branch
from
2314f6b to
05f6511
Compare
openjdk-notifier
Bot
force-pushed
the
pr/804
branch
from
05f6511 to
dd9154b
Compare
|
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8259662-dont-wrap-socketexcpn-8u
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
jerboaa
force-pushed
the
8259662-dont-wrap-socketexcpn-8u
branch
from
0dc5b58 to
fbe874a
Compare
|
@jerboaa Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
Test results of the update (same failures as in the original comment, but 3 more tests added - that pass): |
Can you elaborate on this? These look like two related patches which do the same thing with different exception types. I don't really see a case for saying the risk is fine to change the behaviour for Both also appear to be regressions caused by importing the TLS v1.3 stack into 8u, so it would be a return to 8u's original behaviour at the time of GA, rather than new behaviour. There is an argument that it has now been so long that the current 8u behaviour is the expected behaviour, but again, that's an argument to not do both patches. My current preference would be to do both. It looks like 8239798 was started and nearly integrated a long time ago. I didn't see any issues raised with the risk of it at that time. |
This could be JDK-8384815 which is currently open on PR for 11u. I'll take a look at the status of that one. |
Yes, they both change certain exceptions being thrown/caught. However, the amount of product code change - unrelated to exception handling - in JDK-8239798 is significantly higher than this backport has.
I can look into doing JDK-8239798 as well, but the amount of changes being done to get the wanted changes in is already large and fairly high risk in total (from where I'm standing). If you feel that we should do JDK-8239798 as well, then I'm fine with it. |
See: #825 |
Yes, I noticed that too, but I think most of it is someone decided it would be a good time to also refactor that appear to exist only because the On the positive side, it does bring the code closer to 11u+ but I would also be open to a backport that strips it down to what is needed. But determining that has its own risks. Anyway, we can look at this on the PR you opened. Thanks for doing this.
I am swayed by both being in the Oracle 8u for a long time. On the other hand, I could also go with doing neither. I forgot to ask what this is required for in later backports, as that wasn't clear to me. I am thinking it is overly ambitious to get this all in one release, especially at this late stage, so we should probably lay the foundations in July and do the rest in October. |
Right. That's what it's starting to look like. Anyway, I'll proceed with the backports and the PRs will be there. Please help review! Thanks. Even if it then ends up being pushed one release. |
It's the rename of |
Of course! I'd just feel more comfortable both in my own ability to effectively review them all and the amount of risk in introducing them if some were the other side of rampdown in 8u512. |
jerboaa
force-pushed
the
8259662-dont-wrap-socketexcpn-8u
branch
from
fbe874a to
ee90e96
Compare
|
@jerboaa Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
Re-done on top of #825 now. |
|
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8259662-dont-wrap-socketexcpn-8u
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl
jerboaa
force-pushed
the
8259662-dont-wrap-socketexcpn-8u
branch
from
ee90e96 to
689fa41
Compare
|
@jerboaa Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
Test results still (same as in the PR description): |
|
@gnu-andrew Could you please review! Thank you. |
2 participants
Unclean backport but should be fairly safe in prep for a cleaner backport of JDK-8284047. The patch is not clean because:
jdk/src/share/classes/sun/security/ssl/SSLSocketImpl.javaneeded to be done manually.test/jdk/java/net/httpclient/InvalidSSLContextTest.javanot in 8u. httpclient is 11+SocketExceptioninjdk/test/javax/net/ssl/SSLSession/TestEnabledProtocols.javaIn general the changes are fairly trivial to resolve.
Testing
jdk/test/javax/net/ssljdk/test/sun/net/www/protocol/https/jdk/test/sun/security/ssl/tests. No change tomaster[1].Thoughts?
[1]
Failures/Errors are:
sun/security/ssl/X509KeyManager/PreferredKey.java=> fails in master too. Preexisting.sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java=>Error. Use -nativepath to specify the location of native code. Preexisting.javax/net/ssl/compatibility/Compatibility.java=> fails compilation onUseCase.java. Preexisting.Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/808/head:pull/808$ git checkout pull/808Update a local copy of the PR:
$ git checkout pull/808$ git pull https://git.openjdk.org/jdk8u-dev.git pull/808/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 808View PR using the GUI difftool:
$ git pr show -t 808Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/808.diff
Using Webrev
Link to Webrev Comment