code review 7202045: net: use LockOSThread instead of global mutexes

net: use LockOSThread instead of global mutexes

Global mutexes not only harm concurrency by allowing only
one lookup at a time, but are also misleading: user code
may make identical syscalls from other goroutines without
protection of the same mutex. When lookup code uses its
results it might be executing on another thread and
concurrent calls will overwrite that data.

This change depends on CL 7197050 to work correctly.

[snaury, 2013-01-23 19:58:23]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[brainman, 2013-01-23 22:42:59]

I guess you should change it as per rsc suggestion on your other CL. Thank you.

Alex

[dave_cheney.net, 2013-01-23 22:56:59]

Why can't we use the newLookup method ? That appears to solve all the problems

On Thu, Jan 24, 2013 at 9:42 AM,  <alex.brainman@gmail.com> wrote:
> I guess you should change it as per rsc suggestion on your other CL.
> Thank you.
>
> Alex
>
> https://codereview.appspot.com/7202045/

[brainman, 2013-01-23 23:19:07]

newLookup method uses windows API that is only available on "newer" versions of
OS.

Alex

[dave_cheney.net, 2013-01-23 23:21:13]

i thought i heard someone say xp sp2 and above?

On Thursday, January 24, 2013, wrote:

> newLookup method uses windows API that is only available on "newer"
> versions of OS.
>
> Alex
>
>
https://codereview.appspot.**com/7202045/<https://codereview.appspot.com/7202...
>

[brainman, 2013-01-24 00:10:18]

On 2013/01/23 23:21:13, dfc wrote:
> i thought i heard someone say xp sp2 and above?
> 
> 

That is, probably, what is required, but I am not sure. Go just look for the
function, if it is not there, it falls back to "old ways".

Alex

[rsc, 2013-01-24 02:50:42]

Alex's suggestion SGTM.

[snaury, 2013-01-30 17:15:57]

*** Abandoned ***

[rsc, 2013-01-30 17:28:47]

FWIW, I don't believe anyone is implementing Alex's suggestion (I thought
you would in this CL).

Russ

[snaury, 2013-01-30 17:58:08]

On 2013/01/30 17:28:47, rsc wrote:
> FWIW, I don't believe anyone is implementing Alex's suggestion (I thought
> you would in this CL).

Oh? I didn't think you really meant that. I think spawning a new goroutine would
be an overkill in this case. Not only it might lead to two wasted threads during
the call (in case caller's thread is already locked), you'd need a channel
(which includes a lock) to communicate the result (which might or might not
result in runtime·semacreate which means CreateEvent on Windows), and most
importantly we don't get rid of LockOSThread, we create an ugly workaround due
to inability to know whether's it's locked already or not.

This begs the question, why not just use cgo, which automatically locks only
when necessary? (since it's part of runtime and can see the internals) Except it
seems cgo is not currently used in net package on Windows, for easier
cross-compilation I assume?

[rsc, 2013-01-30 17:59:18]

Alex's suggestion is to figure out whether we are on a new enough system to
use newLookup. If so, use it. If not, use the current slower code (but it's
an ancient system so who cares).

Russ

[snaury, 2013-01-30 18:07:10]

On 2013/01/30 17:59:18, rsc wrote:
> Alex's suggestion is to figure out whether we are on a new enough system to
> use newLookup. If so, use it. If not, use the current slower code (but it's
> an ancient system so who cares).

Isn't it already like that on tip? (see fd_windows.go, function sysInit, if the
new function is available it uses newLookup) But I wasn't creating this CL
because of DNS lookup, I was creating it because the current use of locks is
wrong, it only works by accident, because you don't usually call
syscall.GetProtoByName and syscall.GetServByName directly. If you do call them
directly you may be scheduled to the same thread that did the call in
lookup_windows.go, and then you'll change the buffer that was supposedly
"locked". (which it wasn't, really)

[snaury, 2013-01-30 18:18:55]

On 2013/01/30 17:59:18, rsc wrote:
> Alex's suggestion is to figure out whether we are on a new enough system to
> use newLookup. If so, use it. If not, use the current slower code (but it's
> an ancient system so who cares).

Also, I think we are talking about different things somehow, because earlier
Alex wrote:

"I guess you should change it as per rsc suggestion on your other CL. Thank
you."

And in my other you said:

"It is easy to simulate what you need by launching a new goroutine, having it
lock, and then waiting for it to finish. It's probably not even that much more
expensive."

About which I replied above.

[rsc, 2013-01-30 19:01:45]

Thanks, I didn't realize we already used newLookup where appropriate.

I understand how the old code is 'accidentally' working. I am fine with
leaving the code as is - people who call syscall routines directly must
accept the consequences - and I am fine with putting in an extra goroutine.

I assume this is not actually causing problems for you in real code.

Russ

[snaury, 2013-01-30 19:38:05]

On 2013/01/30 19:01:45, rsc wrote:
> I assume this is not actually causing problems for you in real code.

No, not in real code, but I thought it could cause weird interactions with cgo.
When I tried to make an example:

http://play.golang.org/p/9GVEFDXZDF

It doesn't cause a problem that I thought about though: immediately after
syscall.GetServByName there's not enough gomaxprocs, so the result is scheduled
on another thread, but this thread happens to unlock and get a cgo goroutine in
the mean time, which locks os thread, calls getservbyname and corrupts the port.
For some reason this doesn't happen though, so either my reasoning is wrong or
it's harder than I thought. In any case this would be minor.

[brainman, 2013-01-31 00:24:25]

On 2013/01/30 19:01:45, rsc wrote:
> Thanks, I didn't realize we already used newLookup where appropriate.

We do. My windows xp system uses it, so, I suspect, most users are covered.
Windows 2000 will still runs old code. Maybe some others ...

> ... people who call syscall routines directly must
> accept the consequences ...

True, but ...

> ... and I am fine with putting in an extra goroutine.

this should plug existing hole, and is easy to implement. I am not worried about
speed, given small user base. Also we do not know how slow is it going to be.

I am happy to do it, if no one will.

Alex

[rsc, 2013-01-31 01:22:21]

SGTM

[brainman, 2013-02-01 06:20:09]

On 2013/01/31 00:24:25, brainman wrote:
> On 2013/01/30 19:01:45, rsc wrote:
> > Thanks, I didn't realize we already used newLookup where appropriate.
> 
> We do. ...

Looked at our code and discovered that we still use "old windows socket"
functions in LookupPort and LookupProtocol. Here is a replacement for LookupPort
- https://codereview.appspot.com/7252045/. I do not know an alternative for
GetProtoByName in LookupProtocol. I will, probably, just use "create new thread"
trick for LookupProtocol, since it is rarely called anyway.

Alex

[brainman, 2013-02-04 01:50:29]

On 2013/01/31 01:22:21, rsc wrote:
> SGTM

https://codereview.appspot.com/7293043/

Alex