Project

General

Profile

Actions

Bug #16665

closed

Keepproxy reports error 413 (entity too large) even if the original error was something else

Added by Lucas Di Pentima over 4 years ago. Updated about 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Story points:
-
Release relationship:
Auto

Description

This makes debugging really annoying.


Subtasks 1 (0 open1 closed)

Task #17930: Review 16665-keepproxy-spurious-413-statusResolvedTom Clegg07/22/2021Actions
Actions #1

Updated by Lucas Di Pentima over 3 years ago

Maybe related to this: go-staticcheck (via vscode) is showing me a warning that says:

unreachable case clause: git.arvados.org/arvados.git/sdk/go/keepclient.OversizeBlockError will always match before git.arvados.org/arvados.git/sdk/go/keepclient.InsufficientReplicasError (SA4020)

...for the case keepclient.InsufficientReplicasError: in keepproxy.go's Put() function. Both types are aliases of error so it seems the compiler won't take them as different.

Actions #2

Updated by Peter Amstutz over 3 years ago

type InsufficientReplicasError error

type OversizeBlockError error

    switch err.(type) {
    case nil:
        status = http.StatusOK
        _, err = io.WriteString(resp, locatorOut)

    case keepclient.OversizeBlockError:
        // Too much data
        status = http.StatusRequestEntityTooLarge

    case keepclient.InsufficientReplicasError:
}

I think this is an inconsistency in Go. If you declare a type that is the same as another type, at compile time they will act like different types, but at runtime they will be the same type.

Does this work?

type InsufficientReplicasError struct { error }
Actions #3

Updated by Peter Amstutz over 3 years ago

  • Target version changed from Arvados Future Sprints to 2021-07-21 sprint
Actions #4

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2021-07-21 sprint to 2021-08-04 sprint
Actions #5

Updated by Peter Amstutz over 3 years ago

  • Assigned To set to Lucas Di Pentima
Actions #6

Updated by Lucas Di Pentima over 3 years ago

  • Status changed from New to In Progress
Actions #7

Updated by Lucas Di Pentima over 3 years ago

Updates at e10c23d41 - branch 16665-keepproxy-spurious-413-status
Test run: developer-run-tests: #2606

  • Following Peter's suggestion, updates InsufficientReplicasError & OversizeBlockError error types by wrapping error in a struct, so that the former isn't shadowed by the latter.
  • Updates tests.
Actions #8

Updated by Tom Clegg over 3 years ago

LGTM, thanks.

BTW, about the language issue: error is an interface type, therefore type FooError error defines an interface type, therefore anything that implements the error interface { Error() string } has to match FooError in a type switch. After this fix, type FooError struct { error } is a concrete type, so FooError in a type switch is unambiguous. I think it would have been more normal to use var ErrFoo = errors.New(...) and if err == ErrFoo { ... } for these... oh well.

Actions #9

Updated by Lucas Di Pentima over 3 years ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions #10

Updated by Peter Amstutz about 3 years ago

  • Release set to 42
Actions

Also available in: Atom PDF