bugst / go-serial

A cross-platform serial library for go-lang.
BSD 3-Clause "New" or "Revised" License
619 stars 189 forks source link

Allow Close() to be called from other goroutine than Read() #42

Closed angri closed 4 years ago

angri commented 6 years ago

The only way to interrupt running Read() method of io.Reader is to call Close from another goroutine. What happens is that go runtime builtin race detector complains about unsynchronised access to "opened" member, which is set in Close and checked in Read.

It doesn't seem to be dangerous, but 1) you never know, and 2) it makes your life more difficult when you need to debug some other data races in a complex project that uses serial.

Proposed fix changes simple access to "opened" to atomic operations. It's also supposed to fix #38

Test case is the following:

# unsynced.go
package main

import (
        "context"
        serial "go.bug.st/serial.v1"
        "os/exec"
        "time"
)

func main() {
        ctx, cancel := context.WithCancel(context.Background())
        defer cancel()
        cmd := exec.CommandContext(ctx, "socat", "STDIO", "pty,link=/tmp/faketty")
        if err := cmd.Start(); err != nil {
                panic(err)
        }
        go cmd.Wait()
        // let our fake serial port node to appear
        time.Sleep(time.Millisecond * 100)

        port, err := serial.Open("/tmp/faketty", &serial.Mode{})
        if err != nil {
                panic(err)
        }
        buf := make([]byte, 100)
        go port.Read(buf)
        // let port.Read to start
        time.Sleep(time.Millisecond * 1)
        port.Close()
}

Being run as go run -race unsynced.go it reveals the issue.

niondir commented 6 years ago

@angri would you add also a fix for Windows?

I'm also closing my port on Read timeouts to allow my program to continue with the following close methos in serial_windows.go

type windowsPort struct {
    mu sync.Mutex
    handle syscall.Handle
}

func (port *windowsPort) Close() error {
    port.mu.Lock()
    defer func() {
        port.handle = 0
        port.mu.Unlock()
    }()
    if port.handle == 0 {
        return nil
    }
    return syscall.CloseHandle(port.handle)
}

I'm just using the 0-handle to identify a closed port and a mutex to avoid data races.

graynk commented 5 years ago

@angri would you add also a fix for Windows?

I'm also closing my port on Read timeouts to allow my program to continue with the following close methos in serial_windows.go

type windowsPort struct {
  mu sync.Mutex
  handle syscall.Handle
}

func (port *windowsPort) Close() error {
  port.mu.Lock()
  defer func() {
      port.handle = 0
      port.mu.Unlock()
  }()
  if port.handle == 0 {
      return nil
  }
  return syscall.CloseHandle(port.handle)
}

I'm just using the 0-handle to identify a closed port and a mutex to avoid data races.

A much needed fix, thank you.

cmaglie commented 4 years ago

@angri @Niondir I've manually merged both your contributions. Thank you!