8252767: URLConnection.setRequestProperty throws IllegalAccessError #26

jaikiran · 2020-09-06T05:46:42Z

Can I please get a review and a sponsor for a fix for the issue reported at https://bugs.openjdk.java.net/browse/JDK-8252767?

As noted in that issue, the sun.net.www.URLConnection#setRequestProperty is throwing a IllegalAccessError instead of a IllegalStateException. The commit here fixes that and includes a test which reproduces the issue and verifies the fix.

Would a CSR be needed for this change?

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8252767: URLConnection.setRequestProperty throws IllegalAccessError

Reviewers

Chris Hegarty (@ChrisHegarty - Reviewer)
Michael McMahon (@Michael-Mc-Mahon - Reviewer) ⚠️ Review applies to 9b8539a
Alan Bateman (@AlanBateman - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/26/head:pull/26
$ git checkout pull/26

bridgekeeper · 2020-09-06T05:48:52Z

Hi @jaikiran, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jaikiran" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2020-09-06T05:49:49Z

@jaikiran The following labels will be automatically applied to this pull request: net. When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

jaikiran · 2020-09-06T05:51:47Z

FYI - I am an "author" in the JDK project and have requested for linking this github account to my OpenJDK account (jpai) through this JBS issue https://bugs.openjdk.java.net/browse/SKARA-593

jaikiran · 2020-09-06T08:48:49Z

/solves 8252767

openjdk · 2020-09-06T08:49:18Z

@jaikiran The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

mlbridge · 2020-09-06T08:53:09Z

Webrevs

05: Full - Incremental (0d5d961)
04: Full - Incremental (9b8539a)
03: Full - Incremental (bd64977)
02: Full - Incremental (a819f50c429003976e698c03c862160d88f0d66e)
01: Full - Incremental (faf32fcf5000955e5f1345b530cd8b7ba26b53e6)
00: Full (aad69c9c3ca22e7cb4473ba9738b55796ae070aa)

AlanBateman · 2020-09-06T09:05:55Z

test/jdk/sun/net/www/URLConnectionTest.java

+ * when already connected
+ * @run testng URLConnectionTest
+ */
+public class URLConnectionTest {


I think it would be better to put the test in test/jdk/java/net/URLConnection as its a test for URLConnection.setRequestProperty from the API user point of view. Also it's a a specific test for the behaviour of one method so I think needs a more specific name too ("URLConnectionTest" is too general)

Hello Alan,

There's a test/jdk/java/net/URLConnection/RequestProperties.java test class which tests the setRequestProperty for NullPointerException expectations of that API. Perhaps it would be OK to add this test into that class?

I've now updated this PR to move the test into test/jdk/java/net/URLConnection/RequestProperties.java

AlanBateman · 2020-09-06T10:56:44Z

test/jdk/java/net/URLConnection/RequestProperties.java

+     */
+    private static void testSetRequestPropIllegalStateException() throws Exception {
+        final URL url = new URL("file://" + System.getProperty("java.io.tmpdir"));
+        final java.net.URLConnection conn = url.openConnection();


The URL creation isn't technically right as you can't reliably use a file path string as a URL path. Something like the following would be more correct:
URL url = Path.of(System.getProperty("java.io.tmpdir")).toUri().toURL();

Also minor nit but importing java.net.URL and not URLConnection is a bit strange.

The URL creation isn't technically right as you can't reliably use a file path string as a URL path. Something like the following would be more correct:
URL url = Path.of(System.getProperty("java.io.tmpdir")).toUri().toURL();

I've now updated the PR to fix this.

Also minor nit but importing java.net.URL and not URLConnection is a bit strange.

Sorry, that wasn't intentional. Not sure how I managed to do that. I've included this change in the updated PR.

ChrisHegarty

Overall, this looks good. I added a few minor test related comments.

test/jdk/java/net/URLConnection/RequestProperties.java

ChrisHegarty · 2020-09-07T08:34:25Z

test/jdk/java/net/URLConnection/RequestProperties.java

+        final URLConnection conn = url.openConnection();
+        conn.connect();
+        try {
+            conn.setRequestProperty("foo", "bar");


While here, and not directly related to this change, it would be good to add coverage for the other xxxRequestPropertXXX methods - to ensure that they also throw the correction exception when connected.

Thank you for the review, Chris.

I've updated this PR to include these inputs into the test case.

Summary: Throw an IllegalStateException from sun.net.www.URLConnection#setRequestProperty when already connected Reviewed-By:

ChrisHegarty

LGTM.

openjdk · 2020-09-07T09:15:37Z

@jaikiran This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8252767: URLConnection.setRequestProperty throws IllegalAccessError

Reviewed-by: chegar, michaelm, alanb

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 12 commits pushed to the master branch:

2cceeed: 8166554: Avoid compilation blocking in OverloadCompileQueueTest.java
188b0bc: 8252868: Clean up unused function from G1MMUTracker
891886b: 8252887: Zero VM is broken after JDK-8252661
7686e87: 8250968: Symlinks attributes not preserved when using jarsigner on zip files
8d6d43c: 8251193: bin/idea.sh is generating wrong folder definitions for JVMCI modules
70d5cac: 8251152: ARM32: jtreg c2 Test8202414 test crash
e0d5b5f: 8252627: Make it safe for JFR thread to read threadObj
e29c3f6: 8252661: Change SafepointMechanism terminology to talk less about "blocking"
e0c8d44: 8252844: Update check configuration to Skara format
cc5039e: 8252837: Cleanup SAP Copyright file headers
... and 2 more: https://git.openjdk.java.net/jdk/compare/1262ae36af0c9a41609765966123751b5ad9098b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 2cceeedfe18f699bad4ec5718b0da3fe37364473.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@ChrisHegarty, @Michael-Mc-Mahon, @AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

jaikiran · 2020-09-07T09:19:26Z

/integrate

openjdk · 2020-09-07T09:20:07Z

@jaikiran
Your change (at version bd64977) is now ready to be sponsored by a Committer.

AlanBateman · 2020-09-07T09:24:06Z

test/jdk/java/net/URLConnection/RequestProperties.java

+        } catch (IllegalStateException ise) {
+            // expected
+        }
+    }


Given that the test update is significant then it makes me wonder if we should just move to TestNG while we're at it. It would avoid needing to add expectIllegalStateException.
Minor inconsistency is that is the existing test now has "NPE" as the suffix, the new test "IllegalStateException" as the suffix. Also can we find a better name for "unmetExpectationErrorMessage" as it's not clear to maintainers what "unmet" means.

Given that the test update is significant then it makes me wonder if we should just move to TestNG while we're at it. It would avoid needing to add expectIllegalStateException.

I wasn't sure if it was OK to do it as part of this change. If that's fine, then I will go ahead and change this whole test class into TestNG based test.

AlanBateman · 2020-09-07T09:28:39Z

I see Jaikiran has used "/integrate" but I don't think the review is complete yet. The change may be trivial but it's really important to give others a chance to comment if they wish.

jaikiran · 2020-09-07T09:31:02Z

I see Jaikiran has used "/integrate" but I don't think the review is complete yet. The change may be trivial but it's really important to give others a chance to comment if they wish.

Sorry, I'm new this whole flow. I saw the message from that bot asking me to issue that command. I then went into the "contributing" guidelines to see if there was anything else needed to be done but I didn't see anything specific, so went ahead and issued the /integrate.

I guess if I force push on to this PR, with that TestNG change that you recommended, perhaps the review process will put this PR into the original state which it was before the /integrate command?

jaikiran · 2020-09-07T09:35:16Z

Slightly OT - but I think this /integrate command shouldn't actually be in the hands of the contributor and instead should be decided and issued by "committers" or "sponsors". I don't really see how the contributor/author can realistically decide when it's the right time to intiate a /integrate

ChrisHegarty · 2020-09-07T09:37:23Z

@jaikiran No need to be sorry, you are following the process as it is outlined. Please go ahead with the suggestion to update the test to use TestNG. The current state of the PR should not affect updates. BTW AFAIK, you should not need to "force push" - just push to the branch.

rwestberg · 2020-09-07T09:54:12Z

@jaikiran For what it's worth, this is how the interaction was designed. When an author that isn't an OpenJDK Committer issues the /integrate command, it merely signals that you think the change is ready. If a reviewer disagrees, they will let you know, and you can continue to work on the pull request. Any new commit pushed to your branch will cause the "sponsor" label to be removed.

If the author didn't have to issue the /integrate command we would have the opposite problem - your sponsor can't know that you think that your contribution is really ready - perhaps you have another minor cleanup lined up and are just about to push it. But perhaps we can make this clearer in the message from the bot.

jaikiran · 2020-09-07T11:04:37Z

@jaikiran No need to be sorry, you are following the process as it is outlined. Please go ahead with the suggestion to update the test to use TestNG. The current state of the PR should not affect updates. BTW AFAIK, you should not need to "force push" - just push to the branch.

Thank you Chris. I've now updated this PR by doing a normal push. The updated PR now contains changes to the test, to convert it to TestNG.

Michael-Mc-Mahon

LGTM

ChrisHegarty · 2020-09-07T11:24:18Z

test/jdk/java/net/URLConnection/RequestProperties.java

-            failed++;
-        } catch (NullPointerException e) { /* Expected */ }
+            conn.addRequestProperty(null, "hello");
+            Assert.fail("addRequestProperty on " + conn.getClass().getName()


Now that the test is using TestNG, you can use the convenient Assert.assertThrows / Assert.expectThrows, e.g. something like:

static final Class NPE = NullPointerException.class;

assertThrows(NPE, () -> conn.addRequestProperty(null, "hello"));

similar for IllegalStateException

Thank you. I wasn't aware of those APIs in TestNG. Looks like they got introduced in 6.9.5 of TestNG.
Not sure what version of TestNG, the jtreg tool supports. But when I changed this test to use those APIs and run it, it worked fine and passed. So I guess it's supported.
Using jtreg -version doesn't show the TestNG version (unlike what the docs claim), but that anyway isn't something relevant here and is a question to discuss on the jtreg mailing list:

jtreg -version jtreg, version 5.1 dev 827 Installed in /home/me/jtreg/jtreg/lib/jtreg.jar Running on platform version 1.8.0_265 from /home/me/java/Contents/Home/jre. Built with Java(TM) 2 SDK, Version 1.8.0_172-b11 on June 21, 2020. Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved. Use is subject to license terms. JT Harness, version 6.0 ea b11 (June 21, 2020) JCov 3.0-2 TestNG (testng.jar): version unknown TestNG (jcommander.jar): version 1.72 Java Assembler Tools, version 7.0 beta b02 (June 21, 2020)

I've updated this PR to include your suggested change.

AlanBateman

I think this version looks much better. A few nits with naming/style but not worth spending time on.

dfuch · 2020-09-07T12:41:49Z

Hi Jaikiran,

I'll sponsor this for you when it is ready to integrate but I'd like to send it to our test system first.

best regards,
-- daniel

jaikiran · 2020-09-07T13:48:17Z

Thank you everyone for the reviews and thank you Daniel for sponsoring. I'll wait for an update from you on the test results, to intiate the integrate command.

dfuch · 2020-09-07T15:34:02Z

Hi Jaikiran, the test results came back green.

best regards,

-- daniel

jaikiran · 2020-09-08T01:52:00Z

/integrate

openjdk · 2020-09-08T01:53:00Z

@jaikiran
Your change (at version 0d5d961) is now ready to be sponsored by a Committer.

dfuch · 2020-09-08T09:08:07Z

/sponsor

openjdk · 2020-09-08T09:10:37Z

@dfuch @jaikiran Since your change was applied there have been 12 commits pushed to the master branch:

2cceeed: 8166554: Avoid compilation blocking in OverloadCompileQueueTest.java
188b0bc: 8252868: Clean up unused function from G1MMUTracker
891886b: 8252887: Zero VM is broken after JDK-8252661
7686e87: 8250968: Symlinks attributes not preserved when using jarsigner on zip files
8d6d43c: 8251193: bin/idea.sh is generating wrong folder definitions for JVMCI modules
70d5cac: 8251152: ARM32: jtreg c2 Test8202414 test crash
e0d5b5f: 8252627: Make it safe for JFR thread to read threadObj
e29c3f6: 8252661: Change SafepointMechanism terminology to talk less about "blocking"
e0c8d44: 8252844: Update check configuration to Skara format
cc5039e: 8252837: Cleanup SAP Copyright file headers
... and 2 more: https://git.openjdk.java.net/jdk/compare/1262ae36af0c9a41609765966123751b5ad9098b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5dd1ead.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

It is supposed to be a no-op. As such, the stub files usually omit it. Currently, it actually changes behavior in a bad way: typetools/checker-framework#2995 Until that bug is fixed, this commit serves as a workaround.

@franferrax

…itialized (openjdk#26) Co-authored-by: Martin Balao <mbalao@redhat.com> Reviewed-by: @franferrax, @gnu-andrew

Avoid phi(null, n) from materializing.

bridgekeeper bot added the oca Needs verification of OCA signatory status label Sep 6, 2020

openjdk bot added the net net-dev@openjdk.org label Sep 6, 2020

bridgekeeper bot removed the oca Needs verification of OCA signatory status label Sep 6, 2020

openjdk bot changed the title ~~RFR 8252767: URLConnection.setRequestProperty throws IllegalAccessError~~ 8252767: URLConnection.setRequestProperty throws IllegalAccessError Sep 6, 2020

openjdk bot added the rfr Pull request is ready for review label Sep 6, 2020

AlanBateman reviewed Sep 6, 2020

View reviewed changes

jaikiran force-pushed the 8252767 branch from aad69c9 to faf32fc Compare September 6, 2020 09:34

AlanBateman reviewed Sep 6, 2020

View reviewed changes

jaikiran force-pushed the 8252767 branch from faf32fc to a819f50 Compare September 6, 2020 11:14

ChrisHegarty reviewed Sep 7, 2020

View reviewed changes

8252767: URLConnection.setRequestProperty throws IllegalAccessError

bd64977

Summary: Throw an IllegalStateException from sun.net.www.URLConnection#setRequestProperty when already connected Reviewed-By:

jaikiran force-pushed the 8252767 branch from a819f50 to bd64977 Compare September 7, 2020 09:10

ChrisHegarty approved these changes Sep 7, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 7, 2020

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 7, 2020

AlanBateman reviewed Sep 7, 2020

View reviewed changes

JornVernee mentioned this pull request Sep 7, 2020

Remove mention of /integrate from the start of the pre-integration message. openjdk/skara#801

Closed

2 tasks

Convert to TestNG test case

9b8539a

openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 7, 2020

Michael-Mc-Mahon approved these changes Sep 7, 2020

View reviewed changes

ChrisHegarty reviewed Sep 7, 2020

View reviewed changes

Use new TestNG Assert#assertThrows API

0d5d961

ChrisHegarty approved these changes Sep 7, 2020

View reviewed changes

AlanBateman approved these changes Sep 7, 2020

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 8, 2020

openjdk bot closed this Sep 8, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 8, 2020

asotona pushed a commit to asotona/jdk that referenced this pull request Oct 7, 2022

Fix and test CodeBuilder::swap (openjdk#26)

e76ed5f

asotona pushed a commit to asotona/jdk that referenced this pull request Feb 6, 2023

Fix and test CodeBuilder::swap (openjdk#26)

7e97740

gnu-andrew pushed a commit to gnu-andrew/jdk that referenced this pull request Apr 12, 2023

RH2173781: Avoid calling C_GetInfo() too early, before cryptoki is in…

818c416

…itialized (openjdk#26) Co-authored-by: Martin Balao <mbalao@redhat.com> Reviewed-by: @franferrax, @gnu-andrew

caojoshua pushed a commit to caojoshua/jdk that referenced this pull request May 8, 2023

Merge pull request openjdk#26 from navyxliu/PEA_merge_nullptr

e7a0683

Avoid phi(null, n) from materializing.

robehn pushed a commit to robehn/jdk that referenced this pull request Aug 15, 2023

Push upstream/master to master (openjdk#26)

06f274f

jaikiran deleted the 8252767 branch January 25, 2024 09:38

8252767: URLConnection.setRequestProperty throws IllegalAccessError #26

8252767: URLConnection.setRequestProperty throws IllegalAccessError #26

Conversation

jaikiran commented Sep 6, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 6, 2020

openjdk bot commented Sep 6, 2020

jaikiran commented Sep 6, 2020 • edited by bridgekeeper bot Loading

jaikiran commented Sep 6, 2020

openjdk bot commented Sep 6, 2020

mlbridge bot commented Sep 6, 2020 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 7, 2020 • edited Loading

jaikiran commented Sep 7, 2020

openjdk bot commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman commented Sep 7, 2020

jaikiran commented Sep 7, 2020 • edited Loading

jaikiran commented Sep 7, 2020

ChrisHegarty commented Sep 7, 2020 • edited Loading

rwestberg commented Sep 7, 2020

jaikiran commented Sep 7, 2020

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

dfuch commented Sep 7, 2020

jaikiran commented Sep 7, 2020

dfuch commented Sep 7, 2020

jaikiran commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

dfuch commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

jaikiran commented Sep 6, 2020 •

edited by openjdk bot

Loading

jaikiran commented Sep 6, 2020 •

edited by bridgekeeper bot

Loading

mlbridge bot commented Sep 6, 2020 •

edited

Loading

openjdk bot commented Sep 7, 2020 •

edited

Loading

jaikiran commented Sep 7, 2020 •

edited

Loading

ChrisHegarty commented Sep 7, 2020 •

edited

Loading