Fix read-close deadlock

[cmaglie]

Should fix #13

/cc @larsgk

[larsgk]

Just checked: it doesn't block the serial connection anymore (great job! :))

However, something doesn't get reset - might be on my device side (own HW) .. although works on windows - requiring me to disconnect/connect to get it working again.

[cmaglie]

what is the signal that trigger the reset?

[larsgk]

Not sure - but some other go serial libs seem to remember the settings and reapply after close (might make a difference?) - will try with more hw

On Nov 7, 2016 09:37, "Cristian Maglie" notifications@github.com wrote:

what is the signal that trigger the reset?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bugst/go-serial/pull/14#issuecomment-258775407, or mute the thread https://github.com/notifications/unsubscribe-auth/AA97lkdEggXe8ULpQ4Psua6c_E_zHT5Hks5q7uMugaJpZM4KqnpU .

[larsgk]

Hi @cmaglie ,

I just took at look at the code. The code in the pipe_unix.go file will let the app disconnect - but will (most likely) still keep the serial port side of things open (blocked).

I have seen no immediate problems with the linux part of https://github.com/goburrow/serial - and it has a few things that look important:

https://github.com/goburrow/serial/blob/master/serial_posix.go#L54

and

https://github.com/goburrow/serial/blob/master/serial_posix.go#L74-L83

Thoughts?

[larsgk]

Hi again,

I just did a quick swap to use https://github.com/goburrow/serial and do some stress testing of soft connect/disconnects/reads... and it seems rock solid on linux (the windows impl has problems though).

[cmaglie]

@larsgk restoring the port setting may solve your specific problem, but it's not a thing that I will do in general, I already use the library to just set the serial port modes and I want that modes to be kept after closing the port.

BTW let's find first if the settings are really the issue, and later will decide what to do.

Would you like to try this patch:

https://github.com/cmaglie/go-serial/commit/2fe379734bf390eb2d9edc7f2f0f90fc509b531c

@@ -22,11 +22,15 @@ import (
 type unixPort struct {
    handle int

+   oldSettings *syscall.Termios
+
    closeLock   sync.RWMutex
    closeSignal *pipe
 }

 func (port *unixPort) Close() error {
+   port.setTermSettings(port.oldSettings)
+
    // Close port
    port.releaseExclusiveAccess()
    if err := syscall.Close(port.handle); err != nil {
@@ -109,6 +113,13 @@ func nativeOpen(portName string, mode *Mode) (*unixPort, error) {
        handle: h,
    }

+   oldSettings, err := port.getTermSettings()
+   if err != nil {
+       port.Close()
+       return nil, &PortError{code: InvalidSerialPort}
+   }
+   port.oldSettings = oldSettings
+
    // Setup serial port
    if port.SetMode(mode) != nil {
        port.Close()

[cmaglie]

I just took at look at the code. The code in the pipe_unix.go file will let the app disconnect - but will (most likely) still keep the serial port side of things open (blocked).

I've read some articles about parallel programming with POSIX File Descriptiors, and they seems to agree that closing a file will not unblock functions waiting for a read-ready event (like select or read). Using select to wait for an error-event won't help to unblock either, because closing the port is not an error... It seems that there are no functions that can safely check if a file descriptor has been closed!

The only direct way is to do some I/O to an already closed file descriptor and check for the error, but this still leaves a small race-condition, because the closed file descriptor may be reused in the meantime (so you may end up doing I/O on something totally unrelated with the previous opened file).

In this case the pipe is only used as a trick to unblock the select call, that otherwise will be blocked forever. I'm still trying to find a nice way to avoid the race condition, probably changing port.handle to a known invalid value, like -1, may be a way.

[jcw]

I may have hit the same issue, here's the output I see on MacOS (initial reason for doing the test came from Linux, so it may be the same issue as noted above):

$ go run sertest.go
2016/11/26 00:12:38 begin
2016/11/26 00:12:38 middle
2016/11/26 00:12:38 start read

Then it hangs. The test code is as follows:

package main

import (
    "log"
    "time"

    "go.bug.st/serial.v1"
)

func main() {
    log.Println("begin")
    dev, err := serial.Open("/dev/cu.usbmodemSERPLUS1", &serial.Mode{
        BaudRate: 115200,
    })
    if err != nil {
        log.Println("open?")
        log.Fatal(err)
    }

    go func() {
        log.Println("start read")
        for {
            data := make([]byte, 250)
            if n, err := dev.Read(data); err == nil {
                log.Println("got:", string(data[:n]))
            } else {
                log.Println("read?")
                log.Fatal(err)
            }
        }
        log.Println("finish read")
    }()

    log.Println("middle")
    time.Sleep(2 * time.Second)
    dev.Close()
    time.Sleep(2 * time.Second)
    log.Println("end")
}

But now the crazy bit: I can't kill the process, no ctrl-c, ctrl-\, or even kill -15 or -9 will stop it. Only unplugging the USB device.

-jcw

[cmaglie]

@jcw I tried on mac and indeed the process cannot be killed with -15, BTW I can kill it with a -9. I'm going to merge this PR since it fixes the bad behavior you reported.

@larsgk when you have time to try the snippet of code here https://github.com/bugst/go-serial/pull/14#issuecomment-259115341 please open another issue and report back there!

[larsgk]

@cmaglie ok - I'll try to test it tomorrow - did you already merge? (easier to clone for me anyway ;)) - I had problems making my own fork of our repo work as there are some places expecting the get url to be yours specifically ... my go env was working against me.. maybe you know a trick?

[cmaglie]

This PR is merged, but you still have to manually apply: https://github.com/bugst/go-serial/pull/14#issuecomment-259115341

About development of the library try this: https://github.com/bugst/go-serial#development

basically the trick is to manually create $GOPATH/src/go.bug.st/serial.v1 by forking this repo, the go compiler doesn't care about the origin of the git repo until the path is correct (I mean you can have many git-remote in the repository).

[cmaglie]

Otherwise if you want to make an independent fork you should remove the vanity import lines from each file:

package serial // import "go.bug.st/serial.v1"

should become

package serial

you can do this in a single commit in your master branch and keep merging from my v1 branch. At that point git should take care of the rest.

[larsgk]

cool thanks - I'll try it tomorrow and report back :)

On Sun, Nov 27, 2016 at 9:44 PM, Cristian Maglie notifications@github.com wrote:

Otherwise if you want to make an independent fork you should remove the vanity import lines from each file:

package serial // import "go.bug.st/serial.v1"

should become

package serial

you can do this in a single commit in your master branch and keep merging from my v1 branch. At that point git should take care of the rest.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bugst/go-serial/pull/14#issuecomment-263146254, or mute the thread https://github.com/notifications/unsubscribe-auth/AA97lmOODyn654XLsuWbl3aAPaCkqW6qks5rCeumgaJpZM4KqnpU .