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

8303525: Refactor/cleanup open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java #14932

Closed
wants to merge 7 commits into from

Conversation

mpdonova
Copy link
Contributor

@mpdonova mpdonova commented Jul 19, 2023

This PR refactors the SSLSocketParametersTest by removing redundant/unnecessary classes and cleans up the logic around expected exceptions.


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-8303525: Refactor/cleanup open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java (Task - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14932

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 19, 2023

👋 Welcome back mdonovan! 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 openjdk bot added the rfr Pull request is ready for review label Jul 19, 2023
@openjdk
Copy link

openjdk bot commented Jul 19, 2023

@mpdonova The following label will be automatically applied to this pull request:

  • core-libs

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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 19, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 19, 2023

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 16, 2023

@mpdonova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@mpdonova
Copy link
Contributor Author

I'm still looking for a reviewer for this PR. Thanks!

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 14, 2023

@mpdonova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@mpdonova
Copy link
Contributor Author

Hi, I'm still looking for a reviewer for this PR. Thanks!

serverFactory);
Remote stub = server.runServer();
public void testRmiCommunication(RMIServerSocketFactory serverFactory) throws Exception {
Hello stub = (Hello)UnicastRemoteObject.exportObject(new HelloImpl(),
Copy link

@msheppar msheppar Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by not retaining an explicit reference to the test rmi server, you are exposing it to potentially being GCed during the test execution and potentially prior to client invocation... this might sound fanciful but this has been observed in a few scenarios due to the GC changes ... althought it doesn't seem to have been an issue, the structure of the test appears to be inherently racy, with the potential for the client invocation to getting ahead of the server with the rmi server launching background threads.
Anyways, caution on not retaining a sever reference and GC interference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this can still happen even if the reference to the server object is held in a local variable. It's unlikely to occur, but to prevent any possibility of unexpected GC, you need to use reachabilityFence. Something like:

HelloImpl server = new HelloImpl();
try {
    // ... use server ...
} finally {
    Reference.reachabilityFence(server);
}

I would go ahead and add this out of an abundance of caution.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good advice on reachability fence ... added some -Xcomp args to test runs and no failures detected

@msheppar
Copy link

the test method testServerFactory could be viewed as a misnomer, so refactor rename testSslServerSocketFactory would seem to be more appropriate, while the serverFactory parameter name to testRmiCommunication could likewise be renamed to sslServerSocketFactory (or at least serverSocketFactory).

Copy link
Member

@stuart-marks stuart-marks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanups here. I think investing a bit in the test library will be very helpful. It certainly improves this test, and it opens the possibility of cleaning up other tests as well.

* @param testMethod The code to execute that should throw the exception
* @return The thrown exception.
*/
public static Exception assertThrownException(Class<? extends Exception> expected, TestMethod testMethod) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this is a good addition to the test library, to enable tests to use this style of assertion over testing without having to include JUnit or something. Here I would copy JUnit's expectThrows method signature:

https://github.com/junit-team/junit5/blob/main/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java#L41

That is, make this a generic method with a type variable that extends Throwable, and have the return type be this type variable. You'll probably need to adjust the TestMethod interface to throw Throwable. And I'd rename the method "assertThrows" as well.

The test library doesn't provide all the same overloads for messages as JUnit does. The prevailing style here is to provide an overload that takes a String message and one that doesn't, so following that seems wise.

testMethod.execute();
fail("No exception was thrown. Expected: " + expected.getName());
} catch (Exception exc) {
if (expected.isAssignableFrom(exc.getClass())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use Class.isInstance() here.

return exc;
} else {
fail("An unexpected exception was thrown. Expected: "
+ expected.getName() + " Got: " + exc.getClass().getName());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the fail() overload that takes a Throwable.

}
}
// fail() throws a RuntimeException so this is unreachable.
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this is unfortunate. Even though this method throws along all code paths, the compiler doesn't do this analysis. Even though this "never happens" there's still a question of "but what if?" (I guess it could happen if somebody in the future were to modify the code above.) I'd suggest unconditionally throwing an exception here (probably InternalError) to be more explicit. Returning null will result in an inexplicable NPE if somebody dereferences the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is a mistake. If I call fail() immediately after the test method executes (because an exception wasn't thrown), the RuntimeException which will promptly be caught in my try/catch and that isn't what we want. I'm moving that invocation to the end which looks and works a lot better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Throwing unconditionally at the end of the method is the right thing. Good catch.

Hello stub = (Hello)UnicastRemoteObject.exportObject(server,
0, new SslRMIClientSocketFactory(), serverSocketFactory);
HelloClient client = new HelloClient();
client.runClient(stub);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could even inline HelloClient here, as I don't think that having a separate class is actually doing anything..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does emphasize the classic RMI client/server scenario!

Copy link

@msheppar msheppar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submitted some test runs to mach5 for these changes and they have succeeded

@openjdk
Copy link

openjdk bot commented Oct 18, 2023

@mpdonova 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:

8303525: Refactor/cleanup open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java

Reviewed-by: smarks, msheppar

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 117 new commits pushed to the master branch:

  • e25a49a: 8318471: ProblemList compiler/sharedstubs/SharedTrampolineTest.java
  • ce8ebeb: 8317979: Use TZ database style abbreviations in the CLDR locale provider
  • ab13568: 8318029: Minor improvement to logging output in container at-requires
  • 278de7a: 8318458: Update javac and java manpages with restricted method options
  • 6fc3514: 8318363: Foreign benchmarks fail to build on some platforms
  • 31ef400: 8318183: C2: VM may crash after hitting node limit
  • 4e77b3c: 8315974: Make fields final in 'com.sun.crypto.provider' package
  • 8dd8096: 8317886: Add @sealedGraph to ByteBuffer
  • 9843c97: 8318365: Test runtime/cds/appcds/sharedStrings/InternSharedString.java fails after JDK-8311538
  • 7c80cb2: 8309966: Enhanced TLS connections
  • ... and 107 more: https://git.openjdk.org/jdk/compare/731fb4eea21ab67d90970d7c6107fb0a4fbee9ec...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 openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2023
Copy link
Member

@stuart-marks stuart-marks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@mpdonova
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 23, 2023

Going to push as commit 704c6ea.
Since your change was applied there have been 176 commits pushed to the master branch:

  • 7c0a828: 8318649: G1: Remove unimplemented HeapRegionRemSet::add_code_root_locked
  • ff5c5b6: 8318643: +UseTransparentHugePages must enable +UseLargePages
  • fc29a2e: 8318082: ConcurrentModificationException from IndexWriter
  • 729f4c5: 8318507: G1: Improve remset clearing for humongous candidates
  • 4eab39d: 8318585: Rename CodeCache::UnloadingScope to UnlinkingScope
  • ffadd63: 8317868: Add @sealedGraph to MethodHandleDesc and descendants
  • ecd25e7: 8318484: Initial version of cdsConfig.hpp
  • a876beb: 8316741: BasicStroke.createStrokedShape miter-limits failing on small shapes
  • 4cf195f: 8318573: The nsk.share.jpda.SocketConnection should fail if socket was closed.
  • af2f4bf: 8318622: ProblemList gc/cslocker/TestCSLocker.java on linux-x64 in Xcomp mode
  • ... and 166 more: https://git.openjdk.org/jdk/compare/731fb4eea21ab67d90970d7c6107fb0a4fbee9ec...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 23, 2023
@openjdk openjdk bot closed this Oct 23, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 23, 2023
@openjdk
Copy link

openjdk bot commented Oct 23, 2023

@mpdonova Pushed as commit 704c6ea.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants