On 4 Mar 2015, at 21:02, Stuart Marks <stuart.ma...@oracle.com> wrote:
> On 3/4/15 11:14 AM, Chris Hegarty wrote: >> >>> On 4 Mar 2015, at 18:10, Stuart Marks <stuart.ma...@oracle.com> wrote: >>> >>> Hi Chris, >>> >>> Instead of creating a socket factory for this purpose, this test can use >>> RMI's test library TestLibrary.createRegistryOnUnusedPort(). Now, >>> internally, this uses the now disfavored "getUnusedRandomPort" pattern, but >>> it can (and should) be changed to avoid this. In, fact, passing 0 will >>> work, though it could use its own socket factory if necessary. (It would be >>> good to keep this knowledge within the test library.) >> >> Sorry, I’m confused. Are you suggesting that I change >> TestLibrary.createRegistryOnUnusedPort to use a socket factory, similar to >> the changes in the webrev? Or are you saying that the TestLibrary already >> supports bind to an ephemeral port and subsequently disclosing that port? > > Sorry, there are several pieces moving around here and a couple (or more) > layers of technical debt. > > 1) The test itself should call TestLibrary.createRegistryOnUnusedPort() and > TestLibrary.getRegistryPort(), since those are the abstractions the test > library is attempting to provide. Unfortunately, doing this by itself doesn't > fix the problem, since the test library itself calls getUnusedRandomPort(). > > 2) TestLibrary.createRegistryOnUnusedPort() can be changed to call > LocateRegistry.createRegistry(0), which seems to be permitted by the API and > the implementation, and which works in my limited testing. But it would need > to be tested more thoroughly, and if for some reason it doesn't work, it > could use the socket factory technique or some other technique. Great. This works well, and passes on all platforms. I added TestLibrary.createRegistryOnEphemeralPort(). Eventually other tests using createRegistryOnUnusedPort can be moved over to createRegistryOnEphemeralPort, and then createRegistryOnUnusedPort removed. For now, lets just move PinClientSocketFactory.java and see how it goes. Updated webrev: http://cr.openjdk.java.net/~chegar/8005226/webrev.01/webrev/ Thanks, -Chris > 3) Other RMI tests that create registries will need to be retrofitted to use > the test library in this way. Probably beyond the scope of this changeset. > > I'd prefer not to have the socket factory stuff added to the test, since it'd > just have to be ripped out later when better ephemeral port handling is > supported by the test library. > > 4) If a quick fix is necessary, the test could call > LocateRegistry.createRegistry(0) itself (assuming this works well) if you > don't want to take on the modification of the test library, but this too > would have to be changed eventually. > > So I'd like to see one of the following: > > - both (1) and (2); or > - just (4) > > Your option, depending on how much you want to take on. > > s'marks > > >> >> -Chris. >> >>> The actual port number in use can be fished out of the registry >>> implementation by calling TestLibrary.getRegistryPort(). >>> >>> s'marks >>> >>> >>> On 3/4/15 7:01 AM, Chris Hegarty wrote: >>>> This is a small, test only, review request to fix an intermittently >>>> failing test. >>>> >>>> There is an inherent race, and possible failure, following the >>>> getUnusedRandomPort pattern. This test can be modified to use a custom >>>> socket >>>> factory, supporting listening on an ephemeral port, without changing the >>>> behavior of the test. >>>> >>>> http://cr.openjdk.java.net/~chegar/8005226/webrev.00/webrev/ >>>> >>>> -Chris. >>
