8326568: jdk/test/com/sun/net/httpserver/bugs/B6431193.java should use try-with-resource and try-finally #18514

DarraghClarke · 2024-03-27T14:58:48Z

Currently this test occasionally doesn't cleanup between runs, sometimes not stopping the server or leaving Streams open

Changes:

Use try-with-resources to ensure streams close.
Use try-finally to make sure the server stops before the test exits.

I ran tiers 1-3 and ran this specific test on repeat and everything seems stable after the changes

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326568: jdk/test/com/sun/net/httpserver/bugs/B6431193.java should use try-with-resource and try-finally (Bug - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18514/head:pull/18514
$ git checkout pull/18514

Update a local copy of the PR:
$ git checkout pull/18514
$ git pull https://git.openjdk.org/jdk.git pull/18514/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18514

View PR using the GUI difftool:
$ git pr show -t 18514

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18514.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-27T14:59:35Z

👋 Welcome back dclarke! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-27T14:59:47Z

@DarraghClarke This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8326568: jdk/test/com/sun/net/httpserver/bugs/B6431193.java should use try-with-resource and try-finally

Reviewed-by: dfuchs, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 164 new commits pushed to the master branch:

635cb3c: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions
5c9f036: 8329858: G1: Make G1VerifyLiveAndRemSetClosure stateless
492b954: 8329750: Change Universe functions to return more specific Klass* types
87131fb: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob
5ea21c3: 8329878: Reduce public interface of CardTableBarrierSet
a48289a: 8329761: Remove unused KeyBuilder and unusedSets from StyleContext
8907eda: 8325485: IncrementInstructions.of(int, int) is not storing the args
b9331cd: 8329823: RISC-V: Need to sync CPU features with related JVM flags
71c5bbc: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
58e39c1: 8329884: Serial: Fix build failure due to ‘Copy’ has not been declared
... and 154 more: https://git.openjdk.org/jdk/compare/da8a095a19c90e7ee2b45fab9b533a1092887023...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-03-27T15:00:08Z

@DarraghClarke The following label 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 pull request command.

mlbridge · 2024-03-27T15:02:29Z

Webrevs

dfuch · 2024-03-27T15:11:40Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

-                error = Thread.currentThread().isDaemon();
+                    t.sendResponseHeaders(200, response.length());
+                    os.write(response.getBytes());
+                    os.close();


no need to call os.close() here since it will be called by the try ( ) { } block.

line with os.close() should be removed

Sorry for missing that one on the last PR, removing it now

dfuch · 2024-03-27T15:12:10Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

+                    t.sendResponseHeaders(200, response.length());
+                    os.write(response.getBytes());
+                    os.close();
+                    error = Thread.currentThread().isDaemon();


This error = ... should be moved out of the try () { block, in a } finally { block to make sure the boolean is always set.

dfuch · 2024-03-27T15:14:00Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

            InputStream is = url.openConnection(Proxy.NO_PROXY).getInputStream();
-            read (is);
-            server.stop(0);
+            read(is);


should use try-with-resource here too. read(is) no longer closes is.

Good point, looking at it again would it be worthwhile to get rid of the read method and just use is.readAllBytes in it's place?

Depends on how much data the test actually sends. If it's a small amount readAllBytes is fine.

I think the only data sent is this string: This is the response

jaikiran · 2024-04-01T07:31:08Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

+                    t.sendResponseHeaders(200, response.length());
+                    os.write(response.getBytes());
+                    os.close();
+                    error = Thread.currentThread().isDaemon();


Hello Darragh, since we are updating this test, perhaps we should rename that variable to be a bit more precise? Looking at the history of this test, it was introduced to verify that the HTTP request was handled on the server side in a non-daemon thread. So perhaps rename that field to handlerIsDaemon? Then, the place where we assert this value, perhaps change it to:

if (handlerIsDaemon) { throw new RuntimeException ("request was handled by a daemon thread"); }

DarraghClarke · 2024-04-02T11:52:22Z

Thanks for the suggestions, I've implemented those changes and reran tests to make sure everything is still stable.

dfuch · 2024-04-02T13:23:45Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

            server.setExecutor(null);
            // creates a default executor
-                server.start();
+            server.start();


server.start() (and what preceeds) should be called before entering the try block

dfuch · 2024-04-02T13:25:44Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

            InputStream is = url.openConnection(Proxy.NO_PROXY).getInputStream();
-            read (is);
-            server.stop(0);
-            if (error) {
-                throw new RuntimeException ("error in test");
+            is.readAllBytes();
+            is.close();


you should use try-with-resource here

dfuch · 2024-04-02T13:25:54Z

test/jdk/com/sun/net/httpserver/bugs/B6431193.java

-                error = Thread.currentThread().isDaemon();
+                    t.sendResponseHeaders(200, response.length());
+                    os.write(response.getBytes());
+                    os.close();


line with os.close() should be removed

jaikiran · 2024-04-02T14:59:20Z

Darragh, one other thing - I haven't paid close attention to this test, so can't say for certain - are we sure that when the test code reaches the place where we assert on handlerIsDaemon field, the MyHandler.handle() was indeed invoked? A quick look at that test didn't convince me that this code:

InputStream is = url.openConnection(Proxy.NO_PROXY).getInputStream();
            read (is);

is enough to guarantee that the handler was invoked.

I would have looked at the test code and the HttpURLConnection implementation myself, but I need to head out now.

DarraghClarke · 2024-04-02T15:58:16Z

Darragh, one other thing - I haven't paid close attention to this test, so can't say for certain - are we sure that when the test code reaches the place where we assert on handlerIsDaemon field, the MyHandler.handle() was indeed invoked? A quick look at that test didn't convince me that this code:
InputStream is = url.openConnection(Proxy.NO_PROXY).getInputStream();
            read (is);
is enough to guarantee that the handler was invoked.

I would have looked at the test code and the HttpURLConnection implementation myself, but I need to head out now.

That's a good point, from my testing it seems to consistently be reached but I'm not sure if it's 100% guaranteed. Would a CountDownLatch be useful here maybe, something like this?

} finally {
     handlerIsDaemon = Thread.currentThread().isDaemon();
     latch.countDown();
}

jaikiran · 2024-04-04T11:03:26Z

Hello Darragh,

That's a good point, from my testing it seems to consistently be reached but I'm not sure if it's 100% guaranteed. Would a CountDownLatch be useful here maybe, something like this?

I ran some experiments with this test and it appears that if the handler isn't invoked then we won't reach the place where we assert the handlerIsDaemon field. So I think the current form is OK (of course, the PR needs a couple of updates suggested by Daniel).

Just as an extra precaution, what you could perhaps do is initialize handlerIsDaemon to true instead of the current false. That way it will only be set to false (the expected value) only when the handler is invoked in a non daemon thread.

dfuch

Thanks! LGTM now.

dfuch

Thanks. LGTM now.

jaikiran

Thank you Darragh for the updates. This now looks good to me.

DarraghClarke · 2024-04-10T11:11:47Z

/integrate

openjdk · 2024-04-10T11:12:41Z

Going to push as commit 86cb767.
Since your change was applied there have been 187 commits pushed to the master branch:

b49ba42: 8330002: Remove redundant public keyword in BarrierSet
dd6e453: 8329767: G1: Move G1BlockOffsetTable::set_for_starts_humongous to HeapRegion
e0fd6c4: 8329545: [s390x] Fix garbage value being passed in Argument Register
51ed69a: 8327621: Check return value of uname in os::get_host_name
bea9acc: 8328482: Convert and Open source few manual applet test to main based
d037a59: 8311248: Refactor CodeCache::initialize_heaps to simplify adding new CodeCache segments
bab7019: 8329431: Improve speed of writing CDS heap objects
47df145: 8310513: [s390x] Intrinsify recursive ObjectMonitor locking
b81b86d: 8329729: java/util/Properties/StoreReproducibilityTest.java times out
6276789: 8328785: IOException: Symbol not found: C_GetInterface for PKCS11 interface prior to V3.0
... and 177 more: https://git.openjdk.org/jdk/compare/da8a095a19c90e7ee2b45fab9b533a1092887023...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-10T11:12:47Z

@DarraghClarke Pushed as commit 86cb767.

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

use try-finally and try-with-resources as well as slight cleanup

8e0374e

openjdk bot added the rfr Pull request is ready for review label Mar 27, 2024

openjdk bot added the net net-dev@openjdk.org label Mar 27, 2024

dfuch suggested changes Mar 27, 2024

View reviewed changes

jaikiran reviewed Apr 1, 2024

View reviewed changes

implemented feedback

1294dbc

dfuch suggested changes Apr 2, 2024

View reviewed changes

implementing feedback

c540dd5

dfuch approved these changes Apr 9, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 9, 2024

dfuch approved these changes Apr 9, 2024

View reviewed changes

jaikiran approved these changes Apr 9, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 10, 2024

openjdk bot closed this Apr 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 10, 2024

8326568: jdk/test/com/sun/net/httpserver/bugs/B6431193.java should use try-with-resource and try-finally #18514

8326568: jdk/test/com/sun/net/httpserver/bugs/B6431193.java should use try-with-resource and try-finally #18514

Uh oh!

Conversation

DarraghClarke commented Mar 27, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 27, 2024

Uh oh!

openjdk bot commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 27, 2024

Uh oh!

mlbridge bot commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarraghClarke commented Apr 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarraghClarke commented Apr 2, 2024

Uh oh!

jaikiran commented Apr 4, 2024

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

DarraghClarke commented Apr 10, 2024

Uh oh!

openjdk bot commented Apr 10, 2024

Uh oh!

openjdk bot commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

DarraghClarke commented Mar 27, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 27, 2024 •

edited

Loading

mlbridge bot commented Mar 27, 2024 •

edited

Loading

dfuch Apr 2, 2024 •

edited

Loading

dfuch Mar 27, 2024 •

edited

Loading

dfuch Apr 2, 2024 •

edited

Loading

jaikiran commented Apr 2, 2024 •

edited

Loading