RFR 8005226: TEST_BUG: java/rmi/transport/pinClientSocketFactory/PinClientSocketFactory.java fails intermittently

[Chris Hegarty]

On 4 Mar 2015, at 21:02, Stuart Marks <stuart.marks at oracle.com> wrote:

> On 3/4/15 11:14 AM, Chris Hegarty wrote:
>> 
>>> On 4 Mar 2015, at 18:10, Stuart Marks <stuart.marks at 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.
>>