From 3e93b36ca4d4e08cba8d3f878f4635096cb7411c Mon Sep 17 00:00:00 2001
From: greatroar <@>
Date: Thu, 18 Jun 2020 12:55:29 +0200
Subject: [PATCH 1/6] Make rclone.New private
---
internal/backend/rclone/backend.go | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go
index 339db3bd1..e54819529 100644
--- a/internal/backend/rclone/backend.go
+++ b/internal/backend/rclone/backend.go
@@ -114,7 +114,7 @@ func wrapConn(c *StdioConn, lim limiter.Limiter) wrappedConn {
}
// New initializes a Backend and starts the process.
-func New(cfg Config, lim limiter.Limiter) (*Backend, error) {
+func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) {
var (
args []string
err error
@@ -234,7 +234,7 @@ func New(cfg Config, lim limiter.Limiter) (*Backend, error) {
// Open starts an rclone process with the given config.
func Open(cfg Config, lim limiter.Limiter) (*Backend, error) {
- be, err := New(cfg, lim)
+ be, err := newBackend(cfg, lim)
if err != nil {
return nil, err
}
@@ -260,7 +260,7 @@ func Open(cfg Config, lim limiter.Limiter) (*Backend, error) {
// Create initializes a new restic repo with clone.
func Create(cfg Config) (*Backend, error) {
- be, err := New(cfg, nil)
+ be, err := newBackend(cfg, nil)
if err != nil {
return nil, err
}
From 8554332894d5be615f192298a4f2a1e49b78be30 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Fri, 24 Jul 2020 22:29:37 +0200
Subject: [PATCH 2/6] rclone: Close rclone side of stdio_conn pipes
restic did not notice when the rclone subprocess exited unexpectedly.
restic manually created pipes for stdin and stdout and used these for the
connection to the rclone subprocess. The process creating a pipe gets
file descriptors for the sender and receiver side of a pipe and passes
them on to the subprocess. The expected behavior would be that reads or
writes in the parent process fail / return once the child process dies
as a pipe would now just have a reader or writer but not both.
However, this never happened as restic kept the reader and writer
file descriptors of the pipes. `cmd.StdinPipe` and `cmd.StdoutPipe`
close the subprocess side of pipes once the child process was started
and close the parent process side of pipes once wait has finished. We
can't use these functions as we need access to the raw `os.File` so just
replicate that behavior.
---
internal/backend/rclone/backend.go | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go
index e54819529..5d7e62871 100644
--- a/internal/backend/rclone/backend.go
+++ b/internal/backend/rclone/backend.go
@@ -63,6 +63,9 @@ func run(command string, args ...string) (*StdioConn, *exec.Cmd, *sync.WaitGroup
stdout, w, err := os.Pipe()
if err != nil {
+ // close first pipe
+ r.Close()
+ stdin.Close()
return nil, nil, nil, nil, err
}
@@ -70,6 +73,16 @@ func run(command string, args ...string) (*StdioConn, *exec.Cmd, *sync.WaitGroup
cmd.Stdout = w
bg, err := backend.StartForeground(cmd)
+ // close rclone side of pipes
+ errR := r.Close()
+ errW := w.Close()
+ // return first error
+ if err == nil {
+ err = errR
+ }
+ if err == nil {
+ err = errW
+ }
if err != nil {
return nil, nil, nil, nil, err
}
@@ -183,6 +196,8 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) {
err := cmd.Wait()
debug.Log("Wait returned %v", err)
be.waitResult = err
+ // close our side of the pipes to rclone
+ stdioConn.Close()
close(waitCh)
}()
From bf7b1f12eac0ec2da9aa457ffe9663017c7cd6be Mon Sep 17 00:00:00 2001
From: greatroar <@>
Date: Thu, 18 Jun 2020 13:54:54 +0200
Subject: [PATCH 3/6] rclone: Add test for pipe handling when rclone exits
---
internal/backend/rclone/internal_test.go | 31 ++++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 internal/backend/rclone/internal_test.go
diff --git a/internal/backend/rclone/internal_test.go b/internal/backend/rclone/internal_test.go
new file mode 100644
index 000000000..1705d9baf
--- /dev/null
+++ b/internal/backend/rclone/internal_test.go
@@ -0,0 +1,31 @@
+package rclone
+
+import (
+ "context"
+ "testing"
+
+ "github.com/restic/restic/internal/restic"
+ rtest "github.com/restic/restic/internal/test"
+)
+
+// restic should detect rclone exiting.
+func TestRcloneExit(t *testing.T) {
+ dir, cleanup := rtest.TempDir(t)
+ defer cleanup()
+
+ cfg := NewConfig()
+ cfg.Remote = dir
+ be, err := Open(cfg, nil)
+ rtest.OK(t, err)
+ defer be.Close()
+
+ err = be.cmd.Process.Kill()
+ rtest.OK(t, err)
+ t.Log("killed rclone")
+
+ _, err = be.Stat(context.TODO(), restic.Handle{
+ Name: "foo",
+ Type: restic.DataFile,
+ })
+ rtest.Assert(t, err != nil, "expected an error")
+}
From 3cd927d180d8e514261d4f69f180f3c4d37c9bf0 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Fri, 24 Jul 2020 23:27:47 +0200
Subject: [PATCH 4/6] rclone: Give rclone time to finish before closing stdin
pipe
Calling `Close()` on the rclone backend sometimes failed during test
execution with 'signal: Broken pipe'. The stdio connection closed both
the stdin and stdout file descriptors at the same moment, therefore
giving rclone no chance to properly send any final http2 data frames.
Now the stdin connection to rclone is closed first and will only be
forcefully closed after a timeout. In case rclone exits before the
timeout then the stdio connection will be closed normally.
---
internal/backend/rclone/backend.go | 4 +--
internal/backend/rclone/stdio_conn.go | 37 +++++++++++++++------------
2 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go
index 5d7e62871..c577b0b26 100644
--- a/internal/backend/rclone/backend.go
+++ b/internal/backend/rclone/backend.go
@@ -197,7 +197,7 @@ func newBackend(cfg Config, lim limiter.Limiter) (*Backend, error) {
debug.Log("Wait returned %v", err)
be.waitResult = err
// close our side of the pipes to rclone
- stdioConn.Close()
+ stdioConn.CloseAll()
close(waitCh)
}()
@@ -314,7 +314,7 @@ func (be *Backend) Close() error {
debug.Log("rclone exited")
case <-time.After(waitForExit):
debug.Log("timeout, closing file descriptors")
- err := be.conn.Close()
+ err := be.conn.CloseAll()
if err != nil {
return err
}
diff --git a/internal/backend/rclone/stdio_conn.go b/internal/backend/rclone/stdio_conn.go
index a44e6aa5c..ac8dcc65a 100644
--- a/internal/backend/rclone/stdio_conn.go
+++ b/internal/backend/rclone/stdio_conn.go
@@ -12,10 +12,11 @@ import (
// StdioConn implements a net.Conn via stdin/stdout.
type StdioConn struct {
- stdin *os.File
- stdout *os.File
- cmd *exec.Cmd
- close sync.Once
+ stdin *os.File
+ stdout *os.File
+ cmd *exec.Cmd
+ closeIn sync.Once
+ closeOut sync.Once
}
func (s *StdioConn) Read(p []byte) (int, error) {
@@ -28,21 +29,25 @@ func (s *StdioConn) Write(p []byte) (int, error) {
return n, err
}
-// Close closes both streams.
+// Close closes the stream to the child process.
func (s *StdioConn) Close() (err error) {
- s.close.Do(func() {
- debug.Log("close stdio connection")
- var errs []error
+ s.closeOut.Do(func() {
+ debug.Log("close stdio send connection")
+ err = s.stdout.Close()
+ })
- for _, f := range []func() error{s.stdin.Close, s.stdout.Close} {
- err := f()
- if err != nil {
- errs = append(errs, err)
- }
- }
+ return err
+}
- if len(errs) > 0 {
- err = errs[0]
+// CloseAll closes both streams.
+func (s *StdioConn) CloseAll() (err error) {
+ err = s.Close()
+
+ s.closeIn.Do(func() {
+ debug.Log("close stdio receive connection")
+ err2 := s.stdin.Close()
+ if err == nil {
+ err = err2
}
})
From 01b95814539413162f976447833f15841814ecc8 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Sat, 25 Jul 2020 23:51:02 +0200
Subject: [PATCH 5/6] rclone: Better field names for stdio conn
---
internal/backend/rclone/backend.go | 6 +++---
internal/backend/rclone/stdio_conn.go | 30 +++++++++++++--------------
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/internal/backend/rclone/backend.go b/internal/backend/rclone/backend.go
index c577b0b26..fe371f6a0 100644
--- a/internal/backend/rclone/backend.go
+++ b/internal/backend/rclone/backend.go
@@ -88,9 +88,9 @@ func run(command string, args ...string) (*StdioConn, *exec.Cmd, *sync.WaitGroup
}
c := &StdioConn{
- stdin: stdout,
- stdout: stdin,
- cmd: cmd,
+ receive: stdout,
+ send: stdin,
+ cmd: cmd,
}
return c, cmd, &wg, bg, nil
diff --git a/internal/backend/rclone/stdio_conn.go b/internal/backend/rclone/stdio_conn.go
index ac8dcc65a..af49d79ee 100644
--- a/internal/backend/rclone/stdio_conn.go
+++ b/internal/backend/rclone/stdio_conn.go
@@ -12,28 +12,28 @@ import (
// StdioConn implements a net.Conn via stdin/stdout.
type StdioConn struct {
- stdin *os.File
- stdout *os.File
- cmd *exec.Cmd
- closeIn sync.Once
- closeOut sync.Once
+ receive *os.File
+ send *os.File
+ cmd *exec.Cmd
+ closeRecv sync.Once
+ closeSend sync.Once
}
func (s *StdioConn) Read(p []byte) (int, error) {
- n, err := s.stdin.Read(p)
+ n, err := s.receive.Read(p)
return n, err
}
func (s *StdioConn) Write(p []byte) (int, error) {
- n, err := s.stdout.Write(p)
+ n, err := s.send.Write(p)
return n, err
}
// Close closes the stream to the child process.
func (s *StdioConn) Close() (err error) {
- s.closeOut.Do(func() {
+ s.closeSend.Do(func() {
debug.Log("close stdio send connection")
- err = s.stdout.Close()
+ err = s.send.Close()
})
return err
@@ -43,9 +43,9 @@ func (s *StdioConn) Close() (err error) {
func (s *StdioConn) CloseAll() (err error) {
err = s.Close()
- s.closeIn.Do(func() {
+ s.closeRecv.Do(func() {
debug.Log("close stdio receive connection")
- err2 := s.stdin.Close()
+ err2 := s.receive.Close()
if err == nil {
err = err2
}
@@ -66,8 +66,8 @@ func (s *StdioConn) RemoteAddr() net.Addr {
// SetDeadline sets the read/write deadline.
func (s *StdioConn) SetDeadline(t time.Time) error {
- err1 := s.stdin.SetReadDeadline(t)
- err2 := s.stdout.SetWriteDeadline(t)
+ err1 := s.receive.SetReadDeadline(t)
+ err2 := s.send.SetWriteDeadline(t)
if err1 != nil {
return err1
}
@@ -76,12 +76,12 @@ func (s *StdioConn) SetDeadline(t time.Time) error {
// SetReadDeadline sets the read/write deadline.
func (s *StdioConn) SetReadDeadline(t time.Time) error {
- return s.stdin.SetReadDeadline(t)
+ return s.receive.SetReadDeadline(t)
}
// SetWriteDeadline sets the read/write deadline.
func (s *StdioConn) SetWriteDeadline(t time.Time) error {
- return s.stdout.SetWriteDeadline(t)
+ return s.send.SetWriteDeadline(t)
}
// make sure StdioConn implements net.Conn
From ea97ff1ba4e5d1386b88d6cbda32f48c7fbc8a63 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Sun, 26 Jul 2020 12:06:18 +0200
Subject: [PATCH 6/6] rclone: Skip crash test when rclone is not found
---
internal/backend/rclone/internal_test.go | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/internal/backend/rclone/internal_test.go b/internal/backend/rclone/internal_test.go
index 1705d9baf..435428870 100644
--- a/internal/backend/rclone/internal_test.go
+++ b/internal/backend/rclone/internal_test.go
@@ -2,8 +2,10 @@ package rclone
import (
"context"
+ "os/exec"
"testing"
+ "github.com/restic/restic/internal/errors"
"github.com/restic/restic/internal/restic"
rtest "github.com/restic/restic/internal/test"
)
@@ -16,6 +18,10 @@ func TestRcloneExit(t *testing.T) {
cfg := NewConfig()
cfg.Remote = dir
be, err := Open(cfg, nil)
+ if e, ok := errors.Cause(err).(*exec.Error); ok && e.Err == exec.ErrNotFound {
+ t.Skipf("program %q not found", e.Name)
+ return
+ }
rtest.OK(t, err)
defer be.Close()