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

Error 500? #738

Closed
creighto opened this Issue Sep 16, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@creighto
Copy link

creighto commented Sep 16, 2016

Hey @odeke-em, first of all, thank you so much for maintaining this invaluable Linux client! It has been working great for me for a couple of days now.

But as of today, it seems to have stopped working entirely. I only keep getting the 500 error, e.g.

> drive ls
googleapi: Error 500: Internal Error, internalError

When trying to pull, it actually wants to erase my entire local folder… 💥

> drive pull
Resolving...
googleapi: Error 500: Internal Error, internalError
- {...long list...}
Deletion count 6894 dest: 28.40GB
Proceed with the changes? [Y/n]:

Is there an option to make the output more verbose to debug the situation? -- Thanks.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented Sep 17, 2016

Hello there @creighto, thank you for the bug report and welcome to drive.

Yap, this is a nasty bug, but I got time this afternoon and am about to push up a fix.

@odeke-em odeke-em added the bug label Sep 17, 2016

@odeke-em odeke-em added this to the v0.3.8 milestone Sep 17, 2016

odeke-em added a commit that referenced this issue Sep 17, 2016

propagate and handle remote file resolution errors
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 added a commit that referenced this issue Sep 17, 2016

propagate and handle remote file resolution errors
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

This comment has been minimized.

Copy link
Owner

odeke-em commented Sep 17, 2016

Please get the latest from master, fixed by #741.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.