> On Jan 9, 2017, at 5:45 PM, core-libs-dev-requ...@openjdk.java.net wrote: > > Send core-libs-dev mailing list submissions to > core-libs-dev@openjdk.java.net > > To subscribe or unsubscribe via the World Wide Web, visit > http://mail.openjdk.java.net/mailman/listinfo/core-libs-dev > or, via email, send a message with subject or body 'help' to > core-libs-dev-requ...@openjdk.java.net > > You can reach the person managing the list at > core-libs-dev-ow...@openjdk.java.net > > When replying, please edit your Subject line so it is more specific > than "Re: Contents of core-libs-dev digest..." > > > Today's Topics: > > 1. Re: RFR (JAXP) 8171243 : CatalogManager.catalogResolver > throws FileSystemNotFoundException with jar (Roger Riggs) > 2. Re: RFR: JDK-8172432,jar cleanup/update for module and mrm > jar (Paul Sandoz) > 3. Re: RFR of JDK-8172347: Refactoring > src/java.rmi/share/classes/sun/rmi/registry/RegistryImpl.java to > improve testability of rmiregistry (Hamlin Li) > 4. Re: jdk.internal.reflect.ReflectionFactory and > SecurityManager (Martin Buchholz) > > > ---------------------------------------------------------------------- > > Message: 1 > Date: Mon, 9 Jan 2017 17:17:28 -0500 > From: Roger Riggs <roger.ri...@oracle.com> > To: core-libs-dev@openjdk.java.net > Subject: Re: RFR (JAXP) 8171243 : CatalogManager.catalogResolver > throws FileSystemNotFoundException with jar > Message-ID: <25f1ea5e-a2cd-83c2-8f49-346fc9ff2...@oracle.com> > Content-Type: text/plain; charset=utf-8; format=flowed > > Hi Joe, > > a few comments: > > CatalogManager: > - line 58: "will /return /as no mapping is found"; > Or is it describing the behavior of the CatalogResolver (which is > throw a CatalogException)? > (possible more than 1 place) > > Check the copyrights -> 2017 (JarUtils.java) > > in JAXWS repo: > - Options.java: line 786 - you could use URI[]::new instead of > creating a placeholder URI[0]. > > Looks good, Roger > > >> On 1/9/2017 12:38 PM, huizhe wang wrote: >> Hi, >> >> The current Catalog API accepts file paths or URIs in a form of String >> to create Catalog or CatalogResolver in an effort to maintain >> consistency with the old Catalog API and other existing processors. >> However, that also introduced an ambiguity in the API, which is >> unwanted for a new API in Java SE 9. >> >> Please review the changes. >> In jaxp repo: >> http://cr.openjdk.java.net/~joehw/jdk9/8171243/webrev/ >> >> In jaxws repo: >> http://cr.openjdk.java.net/~joehw/jdk9/8171243_jaxws/webrev/ >> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8171243 >> >> Thanks, >> Joe >> > > > > ------------------------------ > > Message: 2 > Date: Mon, 9 Jan 2017 14:21:48 -0800 > From: Paul Sandoz <paul.san...@oracle.com> > To: Xueming Shen <xueming.s...@oracle.com> > Cc: "core-libs-dev@openjdk.java.net" <core-libs-dev@openjdk.java.net> > Subject: Re: RFR: JDK-8172432,jar cleanup/update for module and mrm > jar > Message-ID: <0c4d9e1d-24f7-4a31-a1b1-b4ab4b9cb...@oracle.com> > Content-Type: text/plain; charset=windows-1252 > > Hi, > > A nice cleanup. > > At this time of year: usual review comment to update the years in the license. > > > Main > ? > > 987 jentries.stream().forEach( je -> > addPackageIfNamed(packages, je)); > > If you wish you can remove the ?.stream()? and go straight to ?.forEach(?)? > on the Set. > > > 1870 private static boolean isModuleInfoEntry(String name) { > 1871 // root or versioned module-info.class > 1872 return name.endsWith(MODULE_INFO) && > 1873 (name.length() == MODULE_INFO.length() || > name.startsWith(VERSIONS_DIR)); > > Is this sufficient? For the versioned case do we need to check it is > VERSIONS_DIR/{n}/MODULE_INFO ? > > > Validator > ? > > 56 private final int vdlen = VERSIONS_DIR.length(); > > Can be static. > > > ConcealedPackage > ? > > Should the class be renamed? > > Paul. > > > >> On 9 Jan 2017, at 10:21, Xueming Shen <xueming.s...@oracle.com> wrote: >> >> Hi, >> >> Please review the following proposed changes for jar tool >> >> issue: https://bugs.openjdk.java.net/browse/JDK-8172432 >> webrev: http://cr.openjdk.java.net/~sherman/8172432/webrev >> http://cr.openjdk.java.net/~sherman/8172432/webrev_top/ >> >> (1) move the "packages" and "jarEntries" from "global" to "local", and only >> collect >> the info when needed (if there is a module-info and we indeed need these >> info >> to update the module-info.class). The goal is to avoid/reduce any possible >> performance >> regression/impact to those"non-module" jar file creation and update >> operations. >> The proposed implementation now collects such info after "expand()" for >> "creation" >> if there is "module-info.class". For "update" it is done during the >> "updating" >> >> (2) consolidate all "validation" related implementation into Validator.java. >> The >> "concealedPkgs" now is generated from the base 'module-info.class" from the >> resulting temporary jar file directly, instead of the "module-info.class" >> binary during >> the creation/update. Again, the reasoning is that the "validation" is only >> needed >> for the mr module jar (for now), it is not needed for a "normal" module jar >> file. >> A clear separation helps reducing the performance impact and improving the >> maintainability. >> >> (3) remove the "checkModuleInfos" logic/implementation, which incorrectly >> enforces >> the restriction such as >> a) there must always be, at least, a root module-info, when there is an >> entry that >> has a name ended up with "module-info.class" in the jar file. >> b) module-info.class file can only be at root or a versioned folder. (why >> I can't jar >> a foo.jar that has a entry module-info.class at an arbitrary place?) >> >> (4) consolidate and share the "updateModuleInfo()" for both creation and >> update. >> >> (5) no longer always copy the "mainClass" and "version" attributes from the >> root >> module-info.class into the corresponding versioned module-info.class >> "silently" >> (in extendedInfoBytes()). Instead, the difference between the root >> module-info.class >> and its versioned copy is checked, jar fails if there is inconsistence. >> >> (6) clean up the Entry class and the expand() implementation. It appears the >> Entry >> class might not be absolutely necessary, but I keep it as is for now. >> >> (7) build the jar with -XDstringConcat=inline flag. >> >> thanks, >> Sherman > > > > ------------------------------ > > Message: 3 > Date: Mon, 9 Jan 2017 14:31:22 -0800 > From: Hamlin Li <huaming...@oracle.com> > To: Roger Riggs <roger.ri...@oracle.com>, core-libs-dev > <core-libs-dev@openjdk.java.net> > Subject: Re: RFR of JDK-8172347: Refactoring > src/java.rmi/share/classes/sun/rmi/registry/RegistryImpl.java to > improve testability of rmiregistry > Message-ID: <81bb8605-3263-46e5-dff2-15c5d9683...@oracle.com> > Content-Type: text/plain; charset=utf-8; format=flowed > > >> On 2017/1/9 13:55, Roger Riggs wrote: >> Hi Hamlin, >> >> Its time to use 2017 for copyright dates. > Hi Roger, > Yes, the new year! :-) >> >> The method name launch does not reflect is really happening. >> Launch implies an active process or thread is being started. >> But in this case there is no active element; it just creates and >> exports a remote object. >> Otherwise, looks fine. > modified as createRegistry. > the code is pushed. > > Thank you > -Hamlin >> >> Thanks, Roger >> >> >>> On 1/9/2017 4:49 PM, Hamlin Li wrote: >>> Hi Roger, >>> >>> Thank you for reviewing, please check the comments inline, and new >>> webrev: http://cr.openjdk.java.net/~mli/8172347/webrev.03/ >>> >>> Thank you >>> -Hamlin >>> >>>> On 2017/1/9 13:04, Roger Riggs wrote: >>>> Hi Hamlin, >>>> >>>> In addition to Mark's comments: >>>> >>>> How about this for the method comment: >>>> >>>> /** >>>> * Return a new RegistryImpl on the requested port and export it to >>>> serve registry requests. >>>> * A classloader is initialized from the system property >>>> "env.class.path" and >>>> * a security manager is set unless one is already set. >>>> * <p> >>>> * The returned Registry is fully functional within the current >>>> process and is usable for >>>> * internal and testing purposes. >>>> * >>>> * @param regPort port on which the rmiregistry accepts requests; >>>> * if 0, an implementation specific port >>>> is assigned >>>> * @return a RegistryImpl instance >>>> * @exception RemoteException If remote operation failed. >>>> * @since 9 >>>> */ >>> Your java doc looks more clear, modified. >>>> public static RegistryImpl run(int regPort) throws >>>> RemoteException {...} >>>> >>>> The method should throw RemoteException. Throws clauses should >>>> always be as specific as possible. >>> modified throws. >>>> >>>>> On 1/9/2017 9:36 AM, Mark Sheppard wrote: >>>>> Hi Hamlin, >>>>> >>>>> If I read the changes correctly, there would appear to be a side >>>>> effect of your refactor extract method "run", such that the static >>>>> member variable >>>>> registry is no longer set in the main method, and is set in the run >>>>> method ? Thus, invoking run multiple times (, whether that is >>>>> intended or not,) will >>>>> see the registry static member variable overwritten, for each call. >>>>> While the current implementation that will occur once only within >>>>> the main method. >>>>> So, what's the desired semantics for multiple "run" calls? >>>> The static registry field is never used and can/should be removed. >>>> It can be handled with a local in the method. >>> handled in a local in the method, and assign it to registry in man. >>>>> >>>>> A "run" method is synonymous with Runnable interface, even though >>>>> this run method has a different signature, it's better >>>>> to help us unwary avoid confusion! >>>>> Also the run method instantiates and returns a RegistryImpl, thus >>>>> exhibiting factory method characteristics. >>>>> As such, perhaps a refactor "rename" would be in order, such as >>>>> create, execute, or launch ? >>>> +1, perhaps createRegistry(int port) >>>> >>> use launch, createRegistry makes me think of it as the same of >>> LocateRegistry.createRegistry. >>>> >>>>> >>>>> It would be helpful to provide a current summary, or a recap, as to >>>>> the motivation for the refactoring. >>>> That can be in the test or the bug report; the implementation code >>>> itself should describe the functionality >>>> and its use. >>> will do it in test. >>>> >>>> $.02, Roger >>>> >>>>> >>>>> Is this complimentary to the constructor RegistryImp(int port) or >>>>> LocateRegistry.createRegistry( ..) ? >>>>> >>>>> >>>>> regards >>>>> Mark >>>>> >>>>> >>>>> >>>>>> On 09/01/2017 07:56, Hamlin Li wrote: >>>>>> Hi Roger, >>>>>> >>>>>> Thank you for reviewing, please check new webrev: >>>>>> http://cr.openjdk.java.net/~mli/8172347/webrev.01/ >>>>>> >>>>>> Thank you >>>>>> -Hamlin >>>>>> >>>>>>> On 2017/1/6 12:56, Roger Riggs wrote: >>>>>>> Hi Hamlin, >>>>>>> >>>>>>> Yes, that looks better. >>>>>>> >>>>>>> On the comments, use the normal javadoc comment conventions for >>>>>>> any public API. >>>>>>> @param @return, @throw, etc. >>>>>>> >>>>>>> I think comments should be direct about what the function does. >>>>>>> It does not need >>>>>>> to explain why so much. Or if so, later and in a separate >>>>>>> paragraph; when reading >>>>>>> the most important information should be first. >>>>>>> >>>>>>> Thanks, Roger >>>>>>> >>>>>>> >>>>>>>> On 1/6/2017 4:59 AM, Hamlin Li wrote: >>>>>>>> >>>>>>>>> On 2017/1/6 6:15, Roger Riggs wrote: >>>>>>>>> Hi Hamlin, >>>>>>>>> >>>>>>>>> There are too many issues being mixed together... >>>>>>>>> >>>>>>>>> Comments on B) RegistryImpl: >>>>>>>>> >>>>>>>>> Refactoring of RegistryImpl Main should be clean and clearly >>>>>>>>> separated. >>>>>>>> Hi Roger, >>>>>>>> >>>>>>>> Thank you for reviewing. >>>>>>>> Not sure if I understood you correctly, I created a new bug to >>>>>>>> track refactoring of RegistryImpl, I will send out separate >>>>>>>> reviews for AltSecurityManager in JDK-8172314, JDK-8030175. >>>>>>>> Please let me know if you did not mean it. >>>>>>>> >>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8172347 >>>>>>>> webrev: http://cr.openjdk.java.net/~mli/8172347/webrev.00/, fix >>>>>>>> all the below points. >>>>>>>> >>>>>>>> Thank you >>>>>>>> -Hamlin >>>>>>>>> >>>>>>>>> 365: static void RegistryImpl run(): >>>>>>>>> >>>>>>>>> - The signature of run should be run(int port) and documented >>>>>>>>> as needing to be run in its >>>>>>>>> own thread since it changes Thread context classloader and >>>>>>>>> that it sets a securityManager. >>>>>>>>> Leave it to main to parse and choose a port number. >>>>>>>>> >>>>>>>>> - The comments should be functional as to the purpose of the >>>>>>>>> code and not referencing a particular bug. >>>>>>>>> (Regardless of prior example). >>>>>>>>> >>>>>>>>> - The comment about getting the exact port is out of place >>>>>>>>> because the port number cannot be >>>>>>>>> retrieved from the returned RegistryImpl. IF that's why >>>>>>>>> this refactoring is needed, then >>>>>>>>> perhaps there should be a getPort method that extracts it >>>>>>>>> from the created UnicastServerRef. >>>>>>>>> >>>>>>>>> 423: static void main(String[] args): >>>>>>>>> >>>>>>>>> - Parsing of args should be left in main(); avoiding the >>>>>>>>> question about why NumberFormat is handled. >>>>>>>>> >>>>>>>>> - Either main or run should set a security manager; but not both. >>>>>>>>> >>>>>>>>> Roger >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> On 1/4/2017 10:21 PM, Hamlin Li wrote: >>>>>>>>>> Hi Roger, >>>>>>>>>> >>>>>>>>>> Thank you for reviewing, please check comments inline. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> On 2017/1/5 4:18, Roger Riggs wrote: >>>>>>>>>>> Hi Hamlin, >>>>>>>>>>> >>>>>>>>>>> The original issue with timeout may be due to heavily loaded >>>>>>>>>>> systems and short timeouts. >>>>>>>>>>> 15 sec is not enough on an overloaded system to wait for a >>>>>>>>>>> process to start and then die. >>>>>>>>>>> >>>>>>>>>>> There is no indication in this issue about port-in-use; that >>>>>>>>>>> would be a different issue. >>>>>>>>>> Agree, I put 2 fixes in one patch together as there is no port >>>>>>>>>> in use issue reported, but by reviewing the code, potential >>>>>>>>>> port in use issue could happen some time in the future. >>>>>>>>> Best to keep 1-1 to avoid complicating the discussion and >>>>>>>>> increasing code complexity. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Common comments to both A and B. >>>>>>>>>>> >>>>>>>>>>> I'll need more time to look at B; it would be cleaner to use >>>>>>>>>>> A if it can address the issue. >>>>>>>>>>> The alternative is to duplicate the code in run() in the >>>>>>>>>>> TestLibrary and not modify the RegistryImpl. >>>>>>>>>> I prefer B, because >>>>>>>>>> 1. Although A looks cleaner but B is simulating more like >>>>>>>>>> rmiregistry. >>>>>>>>>> 2. There are some other issue for example JDK-7146543, >>>>>>>>>> JDK-8030950, JDK-8038772, fix based on version B works well, >>>>>>>>>> but fix based on version A fails. >>>>>>>>>> 3. Impact of RegistryImpl modification is minimal. ( May we >>>>>>>>>> could make Registry run(String args[]) private and access it >>>>>>>>>> in test by reflection? ) >>>>>>>>>> 4. Although it's simple to duplicate the code in run() in >>>>>>>>>> the TestLibrary, but seems it's not a good design choice. >>>>>>>>>> (let's call it version C.) >>>>>>>>>> 5. For JDK-8170728, the fix will need to modify >>>>>>>>>> sun.rmi.registry.RegistryImpl anyway. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thank you for detailed review comments below. >>>>>>>>>> As we have several options, I will wait for your further >>>>>>>>>> comments on choice of version A/B/C, then send out new webrev, >>>>>>>>>> hope I only need to send out one version :-). >>>>>>>>>> >>>>>>>>>> Thank you >>>>>>>>>> -Hamlin >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> JavaVM: >>>>>>>>>>> - Document the new methods. >>>>>>>>>>> >>>>>>>>>>> Line 232: Document the exception that may be thrown from >>>>>>>>>>> exitValue. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> RMID: >>>>>>>>>>> Line 204, 222: when adding new functions to the test library >>>>>>>>>>> please add documentation for the methods. >>>>>>>>>>> There are now many variations of the methods that differ only >>>>>>>>>>> by the number arguments. >>>>>>>>>>> It would be better if the name included some hint as to the >>>>>>>>>>> additional functionality. >>>>>>>>>>> >>>>>>>>>>> typo: "additionalOptions" -> "add*i*tionalOptions" >>>>>>>>>>> >>>>>>>>>>> REGISTRY: >>>>>>>>>>> - Document the new methods. >>>>>>>>>>> >>>>>>>>>>> - The name should be more indicative of its function and >>>>>>>>>>> should NOT be all caps; RMID is an acronym where the caps >>>>>>>>>>> make sense. >>>>>>>>>>> >>>>>>>>>>> - line 105: use JavaVM.waitFor(timeout) and avoid >>>>>>>>>>> duplicating code to wait for the subprocess. >>>>>>>>>>> >>>>>>>>>>> - If the subprocesses are in an unknown state it would be >>>>>>>>>>> useful to print diagnostic info from the subprocess before >>>>>>>>>>> terminating. >>>>>>>>>>> Line 106: >>>>>>>>>>> >>>>>>>>>>> - Line 124: >>>>>>>>>>> - I think I would have promoted the shutdown method to >>>>>>>>>>> JavaVM instead of creating a new cleanup method >>>>>>>>>>> to keep the code simpler. >>>>>>>>>>> >>>>>>>>>>> ** The cleanup method never calls super.cleanup() so the >>>>>>>>>>> process is never destroyed!. >>>>>>>>>>> >>>>>>>>>>> AltSecurityManager.java: >>>>>>>>>>> >>>>>>>>>>> - Line 61: the empty constructor can be removed entirely. >>>>>>>>>>> >>>>>>>>>>> - Line 80: change the message to say the exception did not >>>>>>>>>>> occur. >>>>>>>>>>> As written it implies it may have occurred but was not >>>>>>>>>>> caught. >>>>>>>>>>> >>>>>>>>>>> - Line 86: typo "a unexpected" -> "an unexpected" >>>>>>>>>>> >>>>>>>>>>> - Line 90: remove the printStackTrace; it is not useful and >>>>>>>>>>> is a red flag in .jtr files >>>>>>>>>>> >>>>>>>>>>> - Line 125: I don't see that cleanup is better than destroy; >>>>>>>>>>> If there are doubts about destroy >>>>>>>>>>> then destroy should be fixed not avoided. >>>>>>>>>>> >>>>>>>>>>> Roger >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> On 12/26/2016 3:51 AM, Hamlin Li wrote: >>>>>>>>>>>> Would you please review the below patches? >>>>>>>>>>>> >>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8030175 >>>>>>>>>>>> webrev (version A): >>>>>>>>>>>> http://cr.openjdk.java.net/~mli/8030175/webrev.00/ >>>>>>>>>>>> webrev (version B): >>>>>>>>>>>> http://cr.openjdk.java.net/~mli/8030175/webrev.sun.rmi.registry.RegistryImpl.00/ >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> There are 2 versions to be reviewed. >>>>>>>>>>>> >>>>>>>>>>>> In version A, just use RegistryRunner to replace rmiregistry. >>>>>>>>>>>> In version B, refactor sun.rmi.registry.RegistryImpl to >>>>>>>>>>>> improve the testability of RMI code, and create/use >>>>>>>>>>>> RMIRegistryRunner to simulate rmiregistry. >>>>>>>>>>>> >>>>>>>>>>>> Thank you >>>>>>>>>>>> -Hamlin >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > > > > ------------------------------ > > Message: 4 > Date: Mon, 9 Jan 2017 14:45:45 -0800 > From: Martin Buchholz <marti...@google.com> > To: Paul Sandoz <paul.san...@oracle.com> > Cc: core-libs-dev <core-libs-dev@openjdk.java.net>, > jtreg-...@openjdk.java.net > Subject: Re: jdk.internal.reflect.ReflectionFactory and > SecurityManager > Message-ID: > <CA+kOe0_LEQgjQXw5WvR=bktdjl5n-p0gdxqgevcyqnn1bry...@mail.gmail.com> > Content-Type: text/plain; charset=UTF-8 > > I followed up on my own suggestion and wrote a race-simulating test without > the big hammer: > > /** > * Checks that traversal operations collapse a random pattern of > * dead nodes as could normally only occur with a race. > */ > @Test(dataProvider = "traversalActions") > public void traversalOperationsCollapseNodes( > Consumer<LinkedTransferQueue> traversalAction) { > LinkedTransferQueue q = new LinkedTransferQueue(); > int n = rnd.nextInt(6); > for (int i = 0; i < n; i++) q.add(i); > ArrayList nulledOut = new ArrayList(); > for (Object p = head(q); p != null; p = next(p)) > if (rnd.nextBoolean()) { > nulledOut.add(item(p)); > ITEM.setVolatile(p, null); > } > traversalAction.accept(q); > if (n == 0) > assertEquals(nodeCount(q), 0); > else { > int c = nodeCount(q); > assertEquals(q.size(), c - (q.contains(n - 1) ? 0 : 1)); > for (int i = 0; i < n; i++) > assertTrue(nulledOut.contains(i) ^ q.contains(i)); > } > } > > >> On Mon, Jan 9, 2017 at 2:07 PM, Paul Sandoz <paul.san...@oracle.com> wrote: >> >> >> On 9 Jan 2017, at 13:27, Martin Buchholz <marti...@google.com> wrote: >> >> My whitebox tests tend to use private access only for reading, but I can >> imagine cases where it is useful to give testing code stronger access than >> for regular VarHandles, maybe even to write final fields, for example for >> fuzz testing or reliably creating an internal state that can only happen >> with a race. There's may be a case for providing such a big hammer, if >> it's sufficiently hard to get at, but I can live without it. >> >> >> >> That big hammer could be jdk.internal.misc.Unsafe for certain tightly >> bound tests that really need it. >> >> >> My preference would be to encourage people to use >>> MethodHandles.privateLookupIn. >>> >> >> I'm a convert! >> >> >> Great! >> >> >> It will take a while for privateLookupIn to become "common knowledge". >> >> >> I shall endeavour to mention it at Devoxx US in March where John and I >> will present on java.lang.invoke. >> >> Paul. >> > > > End of core-libs-dev Digest, Vol 117, Issue 23 > **********************************************
