Question about the bug https://bugs.openjdk.java.net/browse/JDK-8031179

Stuart Marks stuart.marks at oracle.com
Thu Jan 30 06:31:56 UTC 2014 

Hi Eric,

Thanks for the updates. I've pushed this changeset:

http://hg.openjdk.java.net/jdk9/dev/jdk/rev/97dc93591ae7

A few comments and minor corrections.

1. There was no changeset in the webrev; it was still just a plain patch. 
Depending on how you ran webrev, you might have stumbled over a bug in webrev. I 
use webrev -r and there's a bug that causes that mode not to generate a 
changeset. I've filed a bug CODETOOLS-7900311 on this and have posted a webrev 
to fix webrev. :-) Meanwhile, it was helpful that you wrote out the changeset 
comments so that I didn't have to.

2. You're a JDK9 Author, so I put your OpenJDK name (ewang) for the changeset 
author and removed the Contributed-by line. (A little-known mercurial fact is 
that anyone can create a changeset and name anyone else as the author of that 
changeset, and push that changeset if they have rights to do so.)

3. I removed some trailing whitespace from the change. The jcheck extension will 
reject changesets that have trailing whitespace (and other issues like tabs, 
malformed changeset comments, invalid usernames, etc.) Henceforth, please make 
sure future patches are jcheck clean.

4. The format string for creating the URL in LookupIPv6.java uses the explicit 
argument index form of the format specifier ("%1$s") which is unnecessary since 
the primary use for this form is to enable reordering of strings for 
localization purposes. I've removed these. In addition, the "port" argument used 
the string format specifier %s instead of the integral numeric format specifier 
%d. Th %s works, but it's surprising. I've corrected this as well.

s'marks


On 1/25/14 1:24 AM, Eric Wang wrote:
> Hi Stuart,
>
> I have made changes based on your comments. Can you please review again? Thanks
> a lot!
> http://cr.openjdk.java.net/~ewang/JDK-8031179/webrev.01/
> <http://cr.openjdk.java.net/%7Eewang/JDK-8031179/webrev.01/>
>
> Eric
> On 2014/1/25 8:53, Stuart Marks wrote:
>> Hi Eric,
>>
>> OK, overall this looks good. There are a few adjustments I'd like you to make
>> before I push it for you. Part of this is to get you to do a more complete job
>> of preparing changesets, and part of it is to make my job as a sponsor easier.
>> :-) Oh, and there a couple style issues as well.
>>
>> 1. In the LookupIPv6 test, REGISTRY_PORT is capitalized, making it appear to
>> be a constant, when in fact it's a variable that contains the result of
>> calling getUnusedRandomPort(). It's also unclear why it needs to be a static.
>> Just make it a local variable with a typical variable name, e.g.
>>
>>     int port = TestLibrary.getUnusedRandomPort();
>>
>> and of course make corresponding name changes where it's used.
>>
>> 2. The creation of the URL string for Naming.lookup() concatenates two string
>> literals "]" + ":". It's probably better to combine these into a single
>> literal "]:". Actually I think it would be preferable to create this URL
>> string using String.format(), so please do this instead.
>>
>> 3. Prepare a complete changeset that's committed locally to your repository
>> before running webrev. This changeset should include properly formatted commit
>> comments, including the bugid line and Reviewed-by line, and optionally a
>> Summary line. Recent versions of webrev will export a mercurial changeset and
>> include it in the webrev output, instead of just diffs. Using the changeset I
>> can just import and push it, instead of having to edit the changeset myself
>> manually.
>>
>> 4. When you post the webrev, you should post updated webrevs under a directory
>> with an incremented sequence number (in this case, webrev.01). That way, links
>> to, and comments on, previous webrevs will not be invalidated.
>>
>> Thanks,
>>
>> s'marks
>>
>> On 1/24/14 1:43 AM, Eric Wang wrote:
>>> Hi Stuart,
>>>
>>> Please review the webrev
>>> http://cr.openjdk.java.net/~ewang/JDK-8031179/webrev.00/, if you are OK with the
>>> changes, could you please be my sponsor?
>>>
>>> Thanks,
>>> Eric
>>> On 2014/1/24 15:14, Eric Wang wrote:
>>>> Hi Stuart,
>>>> Thanks for the suggestion! sorry for reply this mail late as i was busy on
>>>> other tasks
>>>> The webrev has been in the internal review process. Based on the suggestion,
>>>> here is a summary of changes:
>>>>
>>>> 1. Add othervm options to tests:
>>>> java/rmi/Naming/DefaultRegistryPort.java
>>>> java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java
>>>> java/rmi/Naming/LookupNameWithColon.java
>>>> java/rmi/server/UnicastRemoteObject/exportObject/GcDuringExport.java
>>>> sun/rmi/rmic/RMIGenerator/RmicDefault.java
>>>> java/rmi/MarshalledObject/compare/Compare.java
>>>> java/rmi/MarshalledObject/compare/HashCode.java
>>>>
>>>> 2. Remove java/rmi and sun/rmi from othervm.dirs of TEST.ROOT
>>>>
>>>> 3. Filed a another bug https://bugs.openjdk.java.net/browse/JDK-8032629 for
>>>> exclusiveAccess.dirs
>>>>
>>>> Thanks,
>>>> Eric
>>>>
>>>> On 2014/1/18 8:45, Stuart Marks wrote:
>>>>> Hi Eric,
>>>>>
>>>>> Thanks for doing the analysis of the tests that need /othervm. The list of
>>>>> tests that don't need /othervm looks good. One subtlety is this one:
>>>>>
>>>>> java/rmi/activation/ActivationGroupDesc/checkDefaultGroupName/CheckDefaultGroupName.java
>>>>>
>>>>>
>>>>>
>>>>> Most activation tests do need /othervm, but this one doesn't, since all it
>>>>> does is create an ActivationGroupDesc instance, which has no side effects.
>>>>> This is unusual for the activation APIs, since most of them are quite
>>>>> intertwined with the rest of the activation subsystem. So, for this test, the
>>>>> lack of /othervm warrants a comment explaining why /othervm is unnecessary.
>>>>>
>>>>> Regarding merging of the tests java/rmi/MarshalledObject/compare/Compare.java
>>>>> and HashCode.java, this is fairly subtle and not obviously wrong, but it's
>>>>> not obviously right either. Note that different jtreg tests -- even in
>>>>> agentvm or samevm mode -- load the test class in a fresh classloader, which
>>>>> means that the statics are reinitialized. This provides some degree of
>>>>> isolation of the tests even when they're reusing the same JVM. By contrast,
>>>>> calling the two different methods from within the same test exposes the
>>>>> second one to initializations left from the first one. In addition (though
>>>>> I'm not too concerned about this) if the first test fails the second won't be
>>>>> run at all. Merging these tests is kind of out of scope for this particular
>>>>> bug, and since I wasn't fully able to convince myself that the merged tests
>>>>> have the same semantics as the unmerged tests, I'd prefer not to see the
>>>>> merging of these tests as part of this changeset.
>>>>>
>>>>> (Some cleanup of these two tests is probably warranted eventually. I'd take a
>>>>> different approach of merging and refactoring the sources but keeping them as
>>>>> separate test runs, using two @run tags. But we should work on that
>>>>> separately. Meanwhile, the overhead of having these as separate tests is
>>>>> minimal, especially if they're not run in /othervm mode.)
>>>>>
>>>>> I don't think the removal of java/rmi/Naming from exclusiveAccess.dirs is
>>>>> safe at this point. The DefaultRegistryPort.java test uses the default
>>>>> registry port, not a unique one. Conceptually it would need to be converted
>>>>> to use the TestLibrary unique port stuff, but the test is actually about
>>>>> using the default port. So, the solution here isn't obvious.
>>>>>
>>>>> In addition, the java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java
>>>>> test also uses the default registry port. It too will need to converted
>>>>> before java/rmi/Naming is removed from exclusiveAccess.dirs.
>>>>>
>>>>> The java/rmi/Naming/LookupIPv6.java test was converted to use the TestLibrary
>>>>> unique port technique, but only partially. The registry is created on the
>>>>> unique port, but the Naming.lookup() call on the last line of the test
>>>>> doesn't provide a port number, so it does the lookup on the default port
>>>>> instead. This will cause the test to fail in almost all cases.
>>>>>
>>>>> I have to ask, did you run this test with your modifications?
>>>>>
>>>>> (Well, the test would pass if IPv6 is not enabled on the machine running the
>>>>> test, but it only passes because the part of the test that creates and uses
>>>>> the registry is bypassed entirely if IPv6 is not enabled. If you're modifying
>>>>> code, you need to take responsibility for ensuring that the code being
>>>>> modified is actually being run and is doing what you expect.)
>>>>>
>>>>> LookupIPv6.java also needs to have these lines added to the test tags:
>>>>>    * @bug 4402708
>>>>> + * @library ../testlibrary
>>>>> + * @build TestLibrary
>>>>>    * @run main/othervm -Djava.net.preferIPv6Addresses=true LookupIPv6
>>>>> (Their absence will cause the test also to fail in a clean build, but the
>>>>> test will find a TestLibrary class if one had been compiled by a previous
>>>>> test that had required it.)
>>>>>
>>>>> Maybe we should separate the othervm changes (removal of the rmi dirs from
>>>>> othervm.dirs, and addition of /othervm) from the exclusiveAccess.dirs
>>>>> changes. A separate bug would be filed for exclusiveAccess.dirs. I know this
>>>>> means the bug count won't go down, but dealing with exclusiveAccess.dirs
>>>>> means that the logic of various tests will need to be change to use the
>>>>> unique port mechanism. This is a fair chunk of work and it's logically
>>>>> distinct from the othervm work.
>>>>>
>>>>> If we also remove the merging of the MarshalledObject tests, I think we can
>>>>> proceed with the othervm changes.
>>>>>
>>>>> Eric, can you make these changes and send out another webrev?
>>>>>
>>>>> Thanks.
>>>>>
>>>>> s'marks
>>>>>
>>>>>
>>>>>
>>>>> On 1/16/14 7:18 AM, Eric Wang wrote:
>>>>>> Hi Stuart,
>>>>>>
>>>>>> I have generated a webrev for review, can you please help to check whether
>>>>>> the changes are OK.
>>>>>> http://cr.openjdk.java.net/~ewang/JDK-8031179/webrev.00/
>>>>>> <http://cr.openjdk.java.net/%7Eewang/JDK-8031179/webrev.00/>
>>>>>>
>>>>>> Also need your help to confirm that the rest 9 tests in previous mail don't
>>>>>> need the /othervm option indeed.
>>>>>>
>>>>>> Thanks,
>>>>>> Eric
>>>>>> On 2014/1/16 22:55, Eric Wang wrote:
>>>>>>> Hi Stuart,
>>>>>>>
>>>>>>> I'm working on the bug https://bugs.openjdk.java.net/browse/JDK-8031179 to
>>>>>>> add /othervm option to rmi tests, so they can removed from the item
>>>>>>> othervm.dir and exclusiveAccess.dirs.
>>>>>>> By searching the directories java/rmi, sun/rmi and java/rmi/Naming, there
>>>>>>> are 14 tests without othervm option, but only 5 tests need to the /othervm
>>>>>>> tag. they are:
>>>>>>>
>>>>>>> Tests need /othervm option:
>>>>>>> java/rmi/Naming/DefaultRegistryPort.java
>>>>>>> java/rmi/Naming/legalRegistryNames/LegalRegistryNames.java
>>>>>>> java/rmi/Naming/LookupNameWithColon.java
>>>>>>> java/rmi/server/UnicastRemoteObject/exportObject/GcDuringExport.java
>>>>>>> sun/rmi/rmic/RMIGenerator/RmicDefault.java
>>>>>>>
>>>>>>> Tests don't need the /othervm option:
>>>>>>> java/rmi/activation/ActivationGroupDesc/checkDefaultGroupName/CheckDefaultGroupName.java
>>>>>>>
>>>>>>>
>>>>>>> java/rmi/MarshalledObject/compare/Compare.java
>>>>>>> java/rmi/MarshalledObject/compare/HashCode.java
>>>>>>> java/rmi/MarshalledObject/compare/NullReference.java
>>>>>>> java/rmi/server/Unmarshal/PrimitiveClasses.java
>>>>>>> sun/rmi/log/ReliableLog/LogAlignmentTest.java
>>>>>>> sun/rmi/log/ReliableLog/Recovery.java
>>>>>>> sun/rmi/log/ReliableLog/SnapshotSize.java
>>>>>>> sun/rmi/rmic/classpath/RMICClassPathTest.java
>>>>>>>
>>>>>>> Also i have a bit confuse about the test Compare.java and HashCode.java in
>>>>>>> java/rmi/MarshalledObject/compare directory, as they should be merged into
>>>>>>> one test so that we don't need to add additional /othervm option for 2 test.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Eric
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>

More information about the core-libs-dev mailing list