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

Conversation

derElektrobesen
Copy link
Contributor

No description provided.

@erikdubbelboer
Copy link
Collaborator

I think this is a feature other people have requested before. I would like to see some changes to make this more usable before I can accept it.

I think it would be useful to add some documentation on what kind of errors the user can expect as argument to the ErrorHandler function. I think it's error that can be returned by Go while reading from a socket such as io.EOF orio.ErrUnexpectedEOF. It can be ErrBodyTooLarge.
But it can also be errGetOnly, so I think its best if you change errGetOnly to be exported.
Can you also turn this into a properly exported error and add it to the list.
Then I think only these errors remain. I think its best if you turned these into a custom error struct that is exported so users can do a type test for it. Since they contain formatted messages we can't just have an exported error like the others. The custom error struct only has to contain an unexported string and a Error() string function that returns that string.

@derElektrobesen
Copy link
Contributor Author

Thanks for review.

All fixes are done

http.go Outdated
@@ -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

http.go Outdated
@@ -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

server.go Outdated
@@ -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

server.go Outdated
@@ -152,6 +152,17 @@ type Server struct {
// Handler for processing incoming requests.
Handler RequestHandler

// ErrorHandler for returning an error response in user-defined way.
//
// 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

http.go Outdated
@@ -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).
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

@derElektrobesen
Copy link
Contributor Author

Sorry about my english %)

Just I need more practice.

@kirillDanshin kirillDanshin merged commit e771b6f into valyala:master Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants