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

#457: allow to rewrite system error response #458

Merged
merged 3 commits into from
Nov 13, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 24 additions & 7 deletions http.go
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,9 @@ func (req *Request) Read(r *bufio.Reader) error {

const defaultMaxInMemoryFileSize = 16 * 1024 * 1024

var errGetOnly = errors.New("non-GET request received")
// ErrGetOnly is returned when server expects only GET requests,
// but some other type of request came (Server.GetOnly option is enabled).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change enabled to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

var ErrGetOnly = errors.New("non-GET request received")

// ReadLimitBody reads request from the given r, limiting the body size.
//
Expand Down Expand Up @@ -881,7 +883,7 @@ func (req *Request) readLimitBody(r *bufio.Reader, maxBodySize int, getOnly bool
return err
}
if getOnly && !req.Header.IsGet() {
return errGetOnly
return ErrGetOnly
}

if req.MayContinue() {
Expand Down Expand Up @@ -1652,6 +1654,11 @@ func appendBodyFixedSize(r *bufio.Reader, dst []byte, n int) ([]byte, error) {
}
}

// ErrBrokenChunks is returned when server got broken chunked body (Transfer-Encoding: chunked).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to:

ErrBrokenChunks is returned when server receives a broken chunked body (Transfer-Encoding: chunked).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

type ErrBrokenChunks struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being pedantic but can you change this to ErrBrokenChunk. The error is only about one chunk being broken, not multiple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

error
}

func readBodyChunked(r *bufio.Reader, maxBodySize int, dst []byte) ([]byte, error) {
if len(dst) > 0 {
panic("BUG: expected zero-length buffer")
Expand All @@ -1671,7 +1678,9 @@ func readBodyChunked(r *bufio.Reader, maxBodySize int, dst []byte) ([]byte, erro
return dst, err
}
if !bytes.Equal(dst[len(dst)-strCRLFLen:], strCRLF) {
return dst, fmt.Errorf("cannot find crlf at the end of chunk")
return dst, ErrBrokenChunks{
error: fmt.Errorf("cannot find crlf at the end of chunk"),
}
}
dst = dst[:len(dst)-strCRLFLen]
if chunkSize == 0 {
Expand All @@ -1688,23 +1697,31 @@ func parseChunkSize(r *bufio.Reader) (int, error) {
for {
c, err := r.ReadByte()
if err != nil {
return -1, fmt.Errorf("cannot read '\r' char at the end of chunk size: %s", err)
return -1, ErrBrokenChunks{
error: fmt.Errorf("cannot read '\r' char at the end of chunk size: %s", err),
}
}
// Skip any trailing whitespace after chunk size.
if c == ' ' {
continue
}
if c != '\r' {
return -1, fmt.Errorf("unexpected char %q at the end of chunk size. Expected %q", c, '\r')
return -1, ErrBrokenChunks{
error: fmt.Errorf("unexpected char %q at the end of chunk size. Expected %q", c, '\r'),
}
}
break
}
c, err := r.ReadByte()
if err != nil {
return -1, fmt.Errorf("cannot read '\n' char at the end of chunk size: %s", err)
return -1, ErrBrokenChunks{
error: fmt.Errorf("cannot read '\n' char at the end of chunk size: %s", err),
}
}
if c != '\n' {
return -1, fmt.Errorf("unexpected char %q at the end of chunk size. Expected %q", c, '\n')
return -1, ErrBrokenChunks{
error: fmt.Errorf("unexpected char %q at the end of chunk size. Expected %q", c, '\n'),
}
}
return n, nil
}
Expand Down
27 changes: 24 additions & 3 deletions server.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,17 @@ type Server struct {
// Handler for processing incoming requests.
Handler RequestHandler

// ErrorHandler for returning an error response in user-defined way.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to:

ErrorHandler for returning a response in case of an error while receiving or parsing the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

//
// Following errors will be captured with this handler:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I'm not sure if these are really all the errors (I think more io.* errors are possible it's probably better to change this to:

The following is a non-exhaustive list of errors that can be expected as argument:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// * io.EOF
// * io.ErrUnexpectedEOF
// * ErrGetOnly
// * ErrSmallBuffer
// * ErrBodyTooLarge
// * ErrBrokenChunks
ErrorHandler func(ctx *RequestCtx, err error)

// Server name for sending in response headers.
//
// Default server name is used if left blank.
Expand Down Expand Up @@ -1835,7 +1846,7 @@ func (s *Server) serveConn(c net.Conn) error {
if err == io.EOF {
err = nil
} else {
bw = writeErrorResponse(bw, ctx, serverName, err)
bw = s.writeErrorResponse(bw, ctx, serverName, err)
}
break
}
Expand Down Expand Up @@ -1865,7 +1876,7 @@ func (s *Server) serveConn(c net.Conn) error {
br = nil
}
if err != nil {
bw = writeErrorResponse(bw, ctx, serverName, err)
bw = s.writeErrorResponse(bw, ctx, serverName, err)
break
}
}
Expand Down Expand Up @@ -2314,12 +2325,22 @@ func (s *Server) writeFastError(w io.Writer, statusCode int, msg string) {
serverDate.Load(), len(msg), msg)
}

func writeErrorResponse(bw *bufio.Writer, ctx *RequestCtx, serverName []byte, err error) *bufio.Writer {
func defaultErrorHandler(ctx *RequestCtx, err error) {
if _, ok := err.(*ErrSmallBuffer); ok {
ctx.Error("Too big request header", StatusRequestHeaderFieldsTooLarge)
} else {
ctx.Error("Error when parsing request", StatusBadRequest)
}
}

func (s *Server) writeErrorResponse(bw *bufio.Writer, ctx *RequestCtx, serverName []byte, err error) *bufio.Writer {
errorHandler := defaultErrorHandler
if s.ErrorHandler != nil {
errorHandler = s.ErrorHandler
}

errorHandler(ctx, err)

if serverName != nil {
ctx.Response.Header.SetServerBytes(serverName)
}
Expand Down
4 changes: 2 additions & 2 deletions server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2053,8 +2053,8 @@ func TestServerGetOnly(t *testing.T) {
if err == nil {
t.Fatalf("expecting error")
}
if err != errGetOnly {
t.Fatalf("Unexpected error from serveConn: %s. Expecting %s", err, errGetOnly)
if err != ErrGetOnly {
t.Fatalf("Unexpected error from serveConn: %s. Expecting %s", err, ErrGetOnly)
}
case <-time.After(100 * time.Millisecond):
t.Fatalf("timeout")
Expand Down