Skip to content

Commit

Permalink
Allow data store to set the status code for errors
Browse files Browse the repository at this point in the history
Closes #77
  • Loading branch information
Acconut committed Jan 26, 2017
1 parent 675f8fc commit f50f03f
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 45 deletions.
20 changes: 8 additions & 12 deletions metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,14 @@ func (m Metrics) incRequestsTotal(method string) {
// incErrorsTotal increases the counter for this error atomically by one.
func (m Metrics) incErrorsTotal(err error) {
msg := err.Error()
if _, ok := ErrStatusCodes[err]; !ok {
msg = "system error"
}

atomic.AddUint64(m.ErrorsTotal[msg], 1)
if addr, ok := m.ErrorsTotal[msg]; ok {
atomic.AddUint64(addr, 1)
} else {
addr := new(uint64)
*addr = 1
m.ErrorsTotal[msg] = addr

This comment has been minimized.

Copy link
@oliverpool

oliverpool Jan 27, 2017

Contributor

In some corner cases, there might be a race-condition here (two threads writing to the m.ErrorsTotal).

It's not dramatic, since it means that a couple of first errors might no be counted, but it could be solved via a buffered channel used to send all the error messages to one goroutine writer. This goroutine would be the only responsible for updating the map (you could then use a map[string]int for the storage).

I would be happy to make a PR if you wish!

This comment has been minimized.

Copy link
@Acconut

Acconut Jan 27, 2017

Author Member

Good catch. However, instead of using an additional goroutine for this, I believe it's sufficient to use a simple sync.RWMutex for coordinating access. It's less complex, requires no goroutine in the background and seems to offer better performance (see https://groups.google.com/forum/#!topic/golang-nuts/w2XqfcaqIqI but take it with caution).

The question is whether it's more efficient to use a map[string]*int, so we only need the write lock when the error occurs for the first time and use atomic.AddUint64 for increasing the count. Or use a map[string]int but we need the write lock for each update we do, which I believe will result in worse performance as it will present a bottleneck.

What do you think about this?

This comment has been minimized.

Copy link
@oliverpool

oliverpool Jan 27, 2017

Contributor

Or use a map[string]int but we need the write lock for each update we do, which I believe will result in worse performance as it will present a bottleneck.

Good point. Since the errors should be the same at some point, it is probably better to have a quick update (even if the creation might be slower).

Actually a simple Mutex should be enough:

get the ptr
if not got{
  GetLock
  get the ptr
  if not got { // to be sure that the previous lock owner didn't created it)
    create ptr
  }
  ReleaseLock
}
atomically incr ptr

This comment has been minimized.

Copy link
@Acconut

Acconut Jan 27, 2017

Author Member

I don't think your version is free of race-conditions. Even if you have a map where you are only inserting elements (but not deleting them or even updating them), a read must be protected by a lock. @Preetam (who is not involved into this project but has a fantastic blog) has a good post about where he explained how to optimize concurrent map access: https://misfra.me/optimizing-concurrent-map-access-in-go/ The important piece of information is actually the last paragraph where he concludes that you need a RWMutex to ensure safe concurrent access which is still efficient. I really recommend you to read it.

This comment has been minimized.

Copy link
@oliverpool

oliverpool Jan 29, 2017

Contributor

The post is interesting (directly relevant for our discussion ^^), it could be even better with some official source for the claim We can’t guarantee the integrity of the map for readers while there is a writer. (for instance https://golang.org/doc/faq#atomic_maps or https://blog.golang.org/go-maps-in-action#TOC_6.)

This comment has been minimized.

Copy link
@Acconut

Acconut Jan 30, 2017

Author Member

I agree. Some confusion probably arises from the fact that inserting a new entry to a map counts as a write. This statement is phrased a bit ambiguous if you don't know the idea behind it. Anyways, are you still interested in working on this? I would be very pleased if you could help a bit :)

This comment has been minimized.

Copy link
@oliverpool

oliverpool Jan 30, 2017

Contributor

Sure, I would try to do it until the end of this week (before I go away for 2 weeks)

This comment has been minimized.

Copy link
@Acconut

Acconut Jan 30, 2017

Author Member

That would be very nice! Thank you

}
}

// incBytesReceived increases the number of received bytes atomically be the
Expand Down Expand Up @@ -78,13 +81,6 @@ func newMetrics() Metrics {
}

func newErrorsTotalMap() map[string]*uint64 {
m := make(map[string]*uint64, len(ErrStatusCodes)+1)

for err := range ErrStatusCodes {
m[err.Error()] = new(uint64)
}

m["system error"] = new(uint64)

m := make(map[string]*uint64, 20)
return m
}
73 changes: 40 additions & 33 deletions unrouted_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,39 +20,46 @@ var (
reForwardedProto = regexp.MustCompile(`proto=(https?)`)
)

var (
ErrUnsupportedVersion = errors.New("unsupported version")
ErrMaxSizeExceeded = errors.New("maximum size exceeded")
ErrInvalidContentType = errors.New("missing or invalid Content-Type header")
ErrInvalidUploadLength = errors.New("missing or invalid Upload-Length header")
ErrInvalidOffset = errors.New("missing or invalid Upload-Offset header")
ErrNotFound = errors.New("upload not found")
ErrFileLocked = errors.New("file currently locked")
ErrMismatchOffset = errors.New("mismatched offset")
ErrSizeExceeded = errors.New("resource's size exceeded")
ErrNotImplemented = errors.New("feature not implemented")
ErrUploadNotFinished = errors.New("one of the partial uploads is not finished")
ErrInvalidConcat = errors.New("invalid Upload-Concat header")
ErrModifyFinal = errors.New("modifying a final upload is not allowed")
)
// HTTPError represents an error with an additional status code attached
// which may be used when this error is sent in a HTTP response.
// See the net/http package for standardized status codes.
type HTTPError interface {
error
StatusCode() int
}

type httpError struct {
error
statusCode int
}

// HTTP status codes sent in the response when the specific error is returned.
var ErrStatusCodes = map[error]int{
ErrUnsupportedVersion: http.StatusPreconditionFailed,
ErrMaxSizeExceeded: http.StatusRequestEntityTooLarge,
ErrInvalidContentType: http.StatusBadRequest,
ErrInvalidUploadLength: http.StatusBadRequest,
ErrInvalidOffset: http.StatusBadRequest,
ErrNotFound: http.StatusNotFound,
ErrFileLocked: 423, // Locked (WebDAV) (RFC 4918)
ErrMismatchOffset: http.StatusConflict,
ErrSizeExceeded: http.StatusRequestEntityTooLarge,
ErrNotImplemented: http.StatusNotImplemented,
ErrUploadNotFinished: http.StatusBadRequest,
ErrInvalidConcat: http.StatusBadRequest,
ErrModifyFinal: http.StatusForbidden,
func (err httpError) StatusCode() int {
return err.statusCode
}

// NewHTTPError adds the given status code to the provided error and returns
// the new error instance. The status code may be used in corresponding HTTP
// responses. See the net/http package for standardized status codes.
func NewHTTPError(err error, statusCode int) HTTPError {
return httpError{err, statusCode}
}

var (
ErrUnsupportedVersion = NewHTTPError(errors.New("unsupported version"), http.StatusPreconditionFailed)
ErrMaxSizeExceeded = NewHTTPError(errors.New("maximum size exceeded"), http.StatusRequestEntityTooLarge)
ErrInvalidContentType = NewHTTPError(errors.New("missing or invalid Content-Type header"), http.StatusBadRequest)
ErrInvalidUploadLength = NewHTTPError(errors.New("missing or invalid Upload-Length header"), http.StatusBadRequest)
ErrInvalidOffset = NewHTTPError(errors.New("missing or invalid Upload-Offset header"), http.StatusBadRequest)
ErrNotFound = NewHTTPError(errors.New("upload not found"), http.StatusNotFound)
ErrFileLocked = NewHTTPError(errors.New("file currently locked"), 423) // Locked (WebDAV) (RFC 4918)
ErrMismatchOffset = NewHTTPError(errors.New("mismatched offset"), http.StatusConflict)
ErrSizeExceeded = NewHTTPError(errors.New("resource's size exceeded"), http.StatusRequestEntityTooLarge)
ErrNotImplemented = NewHTTPError(errors.New("feature not implemented"), http.StatusNotImplemented)
ErrUploadNotFinished = NewHTTPError(errors.New("one of the partial uploads is not finished"), http.StatusBadRequest)
ErrInvalidConcat = NewHTTPError(errors.New("invalid Upload-Concat header"), http.StatusBadRequest)
ErrModifyFinal = NewHTTPError(errors.New("modifying a final upload is not allowed"), http.StatusForbidden)
)

// UnroutedHandler exposes methods to handle requests as part of the tus protocol,
// such as PostFile, HeadFile, PatchFile and DelFile. In addition the GetFile method
// is provided which is, however, not part of the specification.
Expand Down Expand Up @@ -593,9 +600,9 @@ func (handler *UnroutedHandler) sendError(w http.ResponseWriter, r *http.Request
err = ErrNotFound
}

status, ok := ErrStatusCodes[err]
if !ok {
status = 500
status := 500
if statusErr, ok := err.(HTTPError); ok {
status = statusErr.StatusCode()
}

reason := err.Error() + "\n"
Expand Down

0 comments on commit f50f03f

Please sign in to comment.