Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry named pipe connection on race condition #75

Closed
wants to merge 1 commit into from
Closed

Retry named pipe connection on race condition #75

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions pipe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"net"
"os"
"math/rand"
"syscall"
"time"
"unsafe"
Expand All @@ -21,6 +22,8 @@ import (
//sys localAlloc(uFlags uint32, length uint32) (ptr uintptr) = LocalAlloc

const (
cERROR_FILE_NOT_FOUND = 2
cERROR_BAD_PATHNAME = 161
cERROR_PIPE_BUSY = syscall.Errno(231)
cERROR_NO_DATA = syscall.Errno(232)
cERROR_PIPE_CONNECTED = syscall.Errno(535)
Expand Down Expand Up @@ -62,6 +65,21 @@ type win32MessageBytePipe struct {
readEOF bool
}


func shouldRetry(err error) bool {
if errno, ok := err.(syscall.Errno); ok {
errorCode := int(errno)
if errorCode == cERROR_FILE_NOT_FOUND {
return true
}
if errorCode == cERROR_BAD_PATHNAME {
return true
}
}
return false
}


type pipeAddress string

func (f *win32Pipe) LocalAddr() net.Addr {
Expand Down Expand Up @@ -145,6 +163,13 @@ func DialPipe(path string, timeout *time.Duration) (net.Conn, error) {
var h syscall.Handle
for {
h, err = createFile(path, syscall.GENERIC_READ|syscall.GENERIC_WRITE, 0, nil, syscall.OPEN_EXISTING, syscall.FILE_FLAG_OVERLAPPED|cSECURITY_SQOS_PRESENT|cSECURITY_ANONYMOUS, 0)
if err == nil {
break
}
if shouldRetry(err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to me like it creates an infinite loop if there really is no pipe server present. That's bad.

There needs to be something to catch this. Probably what you could do is move this check to after the timekeeping validation.

time.Sleep(time.Duration(rand.Intn(10)) * time.Millisecond)
continue
}
if err != cERROR_PIPE_BUSY {
break
}
Expand All @@ -159,6 +184,10 @@ func DialPipe(path string, timeout *time.Duration) (net.Conn, error) {
}
err = waitNamedPipe(path, ms)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd have restructured this so that you always loop around to CreateFile.
You don't even need to check for errors at all, because CreateFile does that.

What you do need to do is bail if the time is after the absolute timeout above. (Like 156/181). In fact, If the time out above has expired, you should simply bail, not go down into waitNamedPipe again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put another way, all waitNamedPipe() is doing for you is putting you to sleep a bit so you don't spin hard. The consequences of its error returns are simply not very interesting. In some other project I didn't even use this call at all, and just did a 10 ms sleep between retries...

if shouldRetry(err) {
time.Sleep(time.Duration(rand.Intn(10)) * time.Millisecond)
continue
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we continue here, do we have to close h to avoid a handle leak?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I misunderstood, CreateFile() only allocates a handle when it succeeds and we never reach this path if it does:

for {
    h, err = createFile(path, ...)
    if err == nil {
        break
    }
    ...
    err = waitNamedPipe(path, ...)
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Now I see it (on a bigger screen than my mobile phone, haha :-) ).

}
if err == cERROR_SEM_TIMEOUT {
return nil, ErrTimeout
}
Expand Down