code review 1872045: syscall(windows): defending against GetLastError() retu...

syscall(windows): defending against GetLastError() returns 0 after failed winapi call

[brainman, 2010-07-23 02:36:06]

Hello rsc, Joe Poirier, PeterGo (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2010-07-23 05:17:33]

looks pretty good to me.
should Syscall call SetLastError(0)
before the call, so that we don't return
stale error numbers left over from a
previous, unrelated call?  cgo does this
(clears errno) for C calls on unix.

russ

[brainman, 2010-07-23 05:30:51]

> should Syscall call SetLastError(0)
> before the call, so that we don't return
> stale error numbers left over from a
> previous, unrelated call? 

No, I don't think we need to. If you look at any generated api:

func WSASend(s uint32, bufs *WSABuf, bufcnt uint32, sent *uint32, flags uint32,
overlapped *Overlapped, croutine *byte) (errno int) {
	r1, _, e1 := Syscall9(procWSASend, uintptr(s), uintptr(unsafe.Pointer(bufs)),
uintptr(bufcnt), uintptr(unsafe.Pointer(sent)), uintptr(flags),
uintptr(unsafe.Pointer(overlapped)), uintptr(unsafe.Pointer(croutine)), 0, 0)
	if int(r1) == -1 {
		if e1 != 0 {
			errno = int(e1)
		} else {
			errno = EINVAL
		}
	} else {
		errno = 0
	}
	return
}

we only interested of GetLastError() (e1) on failure. On success, we just set
errno = 0 and return.


Alex

[Joe Poirier, 2010-07-23 06:27:06]

On Fri, Jul 23, 2010 at 12:17 AM, Russ Cox <rsc@golang.org> wrote:
> looks pretty good to me.
> should Syscall call SetLastError(0)
> before the call, so that we don't return
> stale error numbers left over from a
> previous, unrelated call?  cgo does this
> (clears errno) for C calls on unix.
>
> russ
>

I'd have to say yes if for no other reason than to be consistent
with the reason for the CL.

The msdn remarks state that "Most functions call SetLastError
or SetLastErrorEx only when they fail" the operative word being
/most/.

Getting no error code would be just as annoying as getting a
stale error code but the former would be a bit less deceiving.

-joe

[brainman, 2010-07-23 06:42:45]

> Getting no error code would be just as annoying as getting a
> stale error code but the former would be a bit less deceiving.

I didn't even think of that. If windows reports wrong error, I'm quite happy to
pass it on to user as is, instead of translating it to EINVAL. I think calling
SetLastError(0) for every API called is too high price to pay here.


Alex

[rsc, 2010-07-23 06:47:23]

> I didn't even think of that. If windows reports wrong error, I'm quite
> happy to pass it on to user as is, instead of translating it to EINVAL.
> I think calling SetLastError(0) for every API called is too high price
> to pay here.

Do you know how expensive it is?
I don't, but on a typical Unix system the
implementation would be at most a handful
of instructions, which is super cheap compared
to the actual system call.  I think it's probably
worth the cost.  If you get an error return from
a call that doesn't set the errno, you want to get
EINVAL and not some arbitrary number that
was left over from the last error.  That kind of
thing can be very confusing to debug.

If you have a system where you can debug
Windows easily, disassemble SetLastError.
I bet it is very short.

Russ

[brainman, 2010-07-23 06:53:16]

On 2010/07/23 06:47:23, rsc wrote:

> Do you know how expensive it is?

I'm pretty sure, it is very quick. I looked at GetLastError() and that was like
3-4 instructions. It just we're talking about something hypothetical, something
I haven't even seen in life before.

> ...  I think it's probably
> worth the cost.  ...

If you insist. Just let me know.


Alex

[rsc, 2010-07-23 06:57:26]

> I'm pretty sure, it is very quick. I looked at GetLastError() and that
> was like 3-4 instructions. It just we're talking about something
> hypothetical, something I haven't even seen in life before.

I'm pretty sure it will come up, not with CreatePipe but with
something else.  It happens too much on Unix to think that
it will never happen on Windows.  Let's do it.

Russ

[brainman, 2010-07-23 07:31:56]

Hello rsc, Joe Poirier, PeterGo (cc: golang-dev@googlegroups.com),

Please take another look.

[Joe Poirier, 2010-07-23 07:41:56]

LGTM

[peterGo, 2010-07-23 09:42:54]

Russ,



Here's a summary of the pertinent points from the Microsoft Windows
documentation. There are very few exceptions to these rules.



* The only way to determine if a Windows API call succeeds or fails is to use
the return value.



* If it's noted in the documentation for the Windows API function,
SetLastErrorCode sets the last-error code. It may do this when the return value
indicates either success or failure.



* The last-error code, if set, is additional information. It is not a proxy for
the return value.



* The value of the last-error code, even when set by SetLastErrorCode, is
usually not defined. Some functions set the last-error code to zero.



* The value of the last-error code obtained by GetLastError is unreliable. It
may have been overwritten.



Run a simple simulation of basic Go Windows Syscalls by flooding a thread with
go routines, which continously SetLastError and immediately GetLastError,
interleaving writes and reads for the single shared last-error code memory
location. The value of last-error code retrieved by GetLastError sometimes
differs from the value set by the corresponding SetLastError, including
retrieving zero after being set to non-zero. "The last-error code is maintained
on a per-thread basis." 



The Windows port of Go should work in all cases, not just under ideal
conditions.



Peter

[brainman, 2010-07-23 12:17:55]

On 2010/07/23 09:42:54, PeterGo wrote:

> Here's a summary of the pertinent points from the Microsoft Windows
> documentation. There are very few exceptions to these rules.
> 

Do you have a particular suggestion on how to change / improve this particular
CL?


Alex

[peterGo, 2010-07-23 21:34:53]

LGTM

[rsc, 2010-07-25 01:34:27]

> Here's a summary of the pertinent points from the Microsoft Windows
> documentation. There are very few exceptions to these rules.
>
> * The only way to determine if a Windows API call succeeds or fails is
> to use the return value.

Sure.

> * If it's noted in the documentation for the Windows API function,
> SetLastErrorCode sets the last-error code. It may do this when the
> return value indicates either success or failure.

Sure.

> * The last-error code, if set, is additional information. It is not a
> proxy for the return value.

Sure.

> * The value of the last-error code, even when set by SetLastErrorCode,
> is usually not defined. Some functions set the last-error code to zero.

Sure.

> * The value of the last-error code obtained by GetLastError is
> unreliable. It may have been overwritten.

No.  It can only be overwritten by a call on the current thread,
and there are no calls between the one we care about and the
call to GetLastError.  They happen right next to each other,
on a single thread.  There can be no overwriting.

> Run a simple simulation of basic Go Windows Syscalls by flooding a
> thread with go routines, which continously SetLastError and immediately
> GetLastError, interleaving writes and reads for the single shared
> last-error code memory location.

Okay, so don't do that.
It's true that if you start multiple goroutines and they are
multiplexed onto the same Windows thread and they call
SetLastError and GetLastError as you describe, then they will fight
over the last error number.  But that's not what happens here.

What happens here is that when a particular goroutine makes
a system call, it calls GetLastError immediately, before it
has relinquished the cpu to any other goroutine.  There is no
call to GetLastError "later".  There's only one right at the
same time as the system call, with no contention from
other goroutines using the same thread, guaranteed.
The goroutines are synchronized so that only one can
be making a system call in a particular thread at a given time,
and "making a system call" means the process of both
doing the call and calling GetLastError, as one atomic operation.

Russ

[rsc, 2010-07-25 01:34:44]

LGTM

[brainman, 2010-07-26 00:24:56]

*** Submitted as http://code.google.com/p/go/source/detail?r=4a01b8d28570 ***

syscall: improve windows errno handling

R=rsc, Joe Poirier, PeterGo
CC=golang-dev
http://codereview.appspot.com/1872045

Committer: Alex Brainman <alex.brainman@gmail.com>