RFR(S): 8010190: os_windows missing winsock initialization

[David Holmes]

Hi Nils,

On 21/03/2013 12:25 AM, Nils Eliasson wrote:
> Hi David,
>
> Thanks for your feedback!
>
> The lazy sockInit fix is not as ad-hoc as it might look - on all other
> paths a file descriptor is required, i.e. one of the paths with a call
> to initSock has to be called first.
>
> Anyway I took a look at 7034299 and have addressed the race. In the new
> webrev I am removing all the synchronization code and lazy
> initialization, and calling initSock from os::init_2 when
> PrintIdealGraphLevel is used. WinSock will only be initialized when
> needed, and it will be done from single threaded code. If more users of
> WinSock is added later, then we should consider always calling initSock.
>
> I choose not to improve the failure handling. IdealGraphVisualizer is
> currently the only consumer of WinSockAPI (through
> ostream/networkstream) and it is only available in debug/fastdebug. We
> hit asserts if the init has not been run or if the networkstream can't
> connect to the client.

If there is only one consumer of this then how did we suddenly start to 
hit uninitialized paths ? Has that consumer changed which methods it uses?

> New webrev:
> http://cr.openjdk.java.net/~neliasso/7034299/webrev.02/

The code simplification to only initialize from init_2 seems fine.

I'm still a bit concerned about the potential for initialization 
failures though - is there a reason to not simply have initSock return a 
boolean and have os_init2 return JNI_ERR on failure?

Thanks,
David

> Old bug:
> http://http://bugs.sun.com/view_bug.do?bug_id=7034299
>
> Thanks,
> Nils Eliasson
>
> On 2013-03-19 01:51, David Holmes wrote:
>> Hi Nils,
>>
>> You fix is fine as far as it goes but ...
>>
>> ... basically if we make changes as to how this code gets used we will
>> hit new failures until slowly we add an initialization check to all of
>> the methods. This is horribly ad-hoc. Worse there is a race condition
>> (a couple actually) in the initialization code which we are more
>> likely to hit now that we have more paths to this code.
>>
>> Seems to me that the real fix for this code might be to ensure
>> sockInit is called during VM startup instead of lazily? Otherwise, as
>> I said, there's a bunch of race conditions that need fixing anyway.
>>
>> Cheers,
>> David
>>
>> On 18/03/2013 9:57 PM, Nils Eliasson wrote:
>>> Hi all,
>>>
>>> A small change that adds lazy initalization of winsock (os::initsock())
>>> on some paths that needs it. WinSock has only a few uses (streaming
>>> graphs to IdealGraphVisualizers a.f.a.i.k.) so the paths are not hot,
>>> and the initialization will not add any overhead to regular VM use.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~neliasso/8010190/webrev.01/
>>>
>>> Bug:
>>> http://bugs.sun.com/view_bug.do?bug_id=80010190
>>>
>>> Thanks,
>>> Nils Eliasson
>