Bug #16665

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

Added by Lucas Di Pentima about 1 year ago. Updated 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Start date:
07/22/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

This makes debugging really annoying.


Subtasks

Task #17930: Review 16665-keepproxy-spurious-413-statusResolvedTom Clegg

Associated revisions

Revision 68b9c7d3
Added by Lucas Di Pentima 2 months ago

Merge branch '16665-keepproxy-spurious-413-status' into main. Closes #16665

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Lucas Di Pentima 3 months 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.

#2 Updated by Peter Amstutz 3 months 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 }

#3 Updated by Peter Amstutz 3 months ago

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

#4 Updated by Peter Amstutz 3 months ago

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

#5 Updated by Peter Amstutz 2 months ago

  • Assigned To set to Lucas Di Pentima

#6 Updated by Lucas Di Pentima 2 months ago

  • Status changed from New to In Progress

#7 Updated by Lucas Di Pentima 2 months ago

Updates at e10c23d41 - branch 16665-keepproxy-spurious-413-status
Test run: https://ci.arvados.org/job/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.

#8 Updated by Tom Clegg 2 months 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.

#9 Updated by Lucas Di Pentima 2 months ago

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

Also available in: Atom PDF