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.

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

propagate and handle remote file resolution errors #741

Merged

Conversation

odeke-em
Copy link
Owner

Fixes #480.
Fixes #668.
Fixes #728.
Fixes #738.

Ensure that we propagate any remote file resolution errors
during pagination.
Previously pagination was done in a goroutine that just sent
back a channel of files. However, this design meant that
if errors were encountered during the resolution, the channel
would close after just

if err != nil {
  fmt.Println(err)
  break
}

and then close the channel.
Meanwhile the problem then bubbled up to the resolver
that determined which files exist and which don't
thereby triggering deletion operations claiming the remote
files didn't exist say when we Google Drive encountered a 500 or a 504.

This CL fixes these problems by ensuring that we return a paginationPair
which contains two channels:
a) a channel for encountered errors
b) a channel for the actual files as was before.

This then translates to consumers of remote file pagination have to
multiplex on both channels with a select loop and watching if any
errors came through and then propagate that along to the callers
to avoid making a simple bad situation, a total disaster.

  • Exhibits:

To demonstrate the problems with the old and new behavior, let's make a
simulated remote error after resolving a few files, the typical would
be a 500 or 504 from Google Drive.
As you'll see, the old behavior would report a new for a total
addition/deletion which is wrong/faulty behavior causing duplicates
for additions, and spurious additions on remote/local, but also
worse causing deletion of files.

In the new behavior, those problems are trivially handled which is
the correct behavior.

  • Before
diff --git a/src/remote.go b/src/remote.go
index 1219915..6edbf1a 100644
--- a/src/remote.go
+++ b/src/remote.go
@@ -322,6 +322,10 @@ func _reqDoPage(req *drive.FilesListCall, hidden bool, promptOnPagination, nilOn
    iterCount += 1
    fileChan <- NewRemoteFile(f)
      }
+     if iterCount >= 1 {
+   fmt.Printf("err: %v\n", "simulated error")
+   break
+     }
      pageToken = results.NextPageToken
      if pageToken == "" {
$ drive push --verbose vines
Addition count 45 src: 97.43MB
Proceed with the changes? [Y/n]
$ drive pull --verbose vines
Deletion count 45 dest: 97.43MB
Proceed with the changes? [Y/n]:
  • After
diff --git a/src/remote.go b/src/remote.go
index d13b1ff..a6033c7 100644
--- a/src/remote.go
+++ b/src/remote.go
@@ -340,6 +340,11 @@ func _reqDoPage(req *drive.FilesListCall, hidden bool, promptOnPagination, nilOn
    filesChan <- NewRemoteFile(f)
      }

+     if iterCount >= 1 {
+   errsChan <- fmt.Errorf("simulated remote error")
+   break
+     }
+
      pageToken = results.NextPageToken
      if pageToken == "" {
$ drive pull --verbose vines
Resolving...
simulated remote error
$
$ drive push --verbose vines
Resolving...
simulated remote error

Fixes #480.
Fixes #668.
Fixes #728.
Fixes #738.

Ensure that we propagate any remote file resolution errors
during pagination.
Previously pagination was done in a goroutine that just sent
back a channel of files. However, this design meant that
if errors were encountered during the resolution, the channel
would close after just
```go
if err != nil {
  fmt.Println(err)
  break
}
```

and then close the channel.
Meanwhile the problem then bubbled up to the resolver
that determined which files exist and which don't
thereby triggering deletion operations claiming the remote
files didn't exist say when we Google Drive encountered a 500 or a 504.

This CL fixes these problems by ensuring that we return a paginationPair
which contains two channels:
a) a channel for encountered errors
b) a channel for the actual files as was before.

This then translates to consumers of remote file pagination have to
multiplex on both channels with a select loop and watching if any
errors came through and then propagate that along to the callers
to avoid making a simple bad situation, a total disaster.

* Exhibits:

To demonstrate the problems with the old and new behavior, let's make a
simulated remote error after resolving a few files, the typical would
be a 500 or 504 from Google Drive.
As you'll see, the old behavior would report a new for a total
addition/deletion which is wrong/faulty behavior causing duplicates
for additions, and spurious additions on remote/local, but also
worse causing deletion of files.

In the new behavior, those problems are trivially handled which is
the correct behavior.

+ Before
```diff
diff --git a/src/remote.go b/src/remote.go
index 1219915..6edbf1a 100644
--- a/src/remote.go
+++ b/src/remote.go
@@ -322,6 +322,10 @@ func _reqDoPage(req *drive.FilesListCall, hidden bool, promptOnPagination, nilOn
	iterCount += 1
	fileChan <- NewRemoteFile(f)
      }
+     if iterCount >= 1 {
+	fmt.Printf("err: %v\n", "simulated error")
+	break
+     }
      pageToken = results.NextPageToken
      if pageToken == "" {
```

```shell
$ drive push --verbose vines
Addition count 45 src: 97.43MB
Proceed with the changes? [Y/n]
$ drive pull --verbose vines
Deletion count 45 dest: 97.43MB
Proceed with the changes? [Y/n]:
```

+ After
```diff
diff --git a/src/remote.go b/src/remote.go
index d13b1ff..a6033c7 100644
--- a/src/remote.go
+++ b/src/remote.go
@@ -340,6 +340,11 @@ func _reqDoPage(req *drive.FilesListCall, hidden bool, promptOnPagination, nilOn
	filesChan <- NewRemoteFile(f)
      }

+     if iterCount >= 1 {
+	errsChan <- fmt.Errorf("simulated remote error")
+	break
+     }
+
      pageToken = results.NextPageToken
      if pageToken == "" {
```

```shell
$ drive pull --verbose vines
Resolving...
simulated remote error
$
$ drive push --verbose vines
Resolving...
simulated remote error
```
@odeke-em odeke-em force-pushed the propagate-and-handle-remote-errors-during-resolution branch from adb1b97 to 0d1e5f8 Compare September 17, 2016 22:51
@odeke-em odeke-em merged commit 0d1e5f8 into master Sep 17, 2016
@odeke-em odeke-em deleted the propagate-and-handle-remote-errors-during-resolution branch September 17, 2016 23:15
odeke-em added a commit that referenced this pull request Jul 24, 2017
Ensure that nil is properly sent back after remote
change/file resolution. This regression was caused
by forgetting to send back that nil.

This regression was caused by PR
#741.

The consequence of the bug was that trying to push
from non-existent folders would erraneously give
back
```shell
Resolving...
Everything is up-to-date.
```

since the remotesChan channel would get closed
after resolution without sending notifying
whoever was resolving that the file or parent didn't
exist remotely.

Fixes #933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant