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

Replace HTTP::Server::Response#respond_with_error with #respond_with_status #6988

Merged

Conversation

@straight-shoota
Copy link
Member

commented Oct 24, 2018

Response#respond_with_error would previously just write the error header and flush the IO.
That flush seems unnecessary. Instead the response should be closed immediately. A call to this method was typically followed by a call tall to close anyway (which is no longer needed with this PR)

The only benefit of not closing directly is that you could append to the body (for example to provide more error data). That's a pretty irrelevant use case however. #respond_with_error already writes a body and you can't replace it, only append. Hence custom error pages will need to a custom implementation anyway.

This also changes the error response to a fixed content length.

/cc @jhass

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

It buries something that used to be explicit in the request handler. Sending an error response and deciding to close the connection are different matters.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

It doesn't close the connection, just the response. #responde_with_error sends a complete response, so it should be closed.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

It could be considered to rename the method to #close_with_error to make the behaviour more explicit.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from e44d5a9 to 219c048 Dec 6, 2018
@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from 219c048 to db7be67 Aug 1, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Rebased onto master.

src/http/server/response.cr Show resolved Hide resolved
@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from db7be67 to 3025db6 Aug 1, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

I added the closing behaviour to the method documentation.

While at it, I also added an overload accepting HTTP::Status, which is a nice convenience.

# Calls `reset` and then writes the given message.
# Calls `#reset`, writes the given status, and closes the response.
def respond_with_error(status : HTTP::Status)
respond_with_error(status.description, status.code)

This comment has been minimized.

Copy link
@Sija

Sija Aug 2, 2019

Contributor

Minor optimization: I'd use this method as the main impl, since this way you could avoid allocating HTTP::Status twice.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Aug 2, 2019

Author Member

I thought about that, but HTTP::Status is an enum, so it's actually not an allocation, just a cast and some validation. In order to provide a custom error message, this would need an additional message argument as well. But this should be the second (optional) argument, which would be inverse to the existing method.
However, that might really be the better API.
Also, I figured this method should perhaps be renamed. After all, it is not only useful for errors, but can send any kind of status code.
So my suggestion would be to change the API to this:

def respond_with_status(status : HTTP::Status, message : String? = nil)
  message || = status.description
  # impl
end

def respond_with_status(status : Int, message : String? = nil)
  respond_with_status(HTTP::Status.new(status), message)
end

This comment has been minimized.

Copy link
@Sija

Sija Aug 2, 2019

Contributor

Sounds like a nice improvement!

message ||= @status.description
self.content_type = "text/plain"
self << @status.code << ' ' << message << '\n'
close
end

# :ditto:
def respond_with_status(status : Int = 500, message : String? = nil)

This comment has been minimized.

Copy link
@Sija

Sija Aug 3, 2019

Contributor

status argument IMO should be required.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Aug 3, 2019

Author Member

Yeah, I guess that's better. Before, it was respond_with_error where one could argue that a default error status is 500. But even that's not necessarily the case. So, yeah, I'll make it required.

@straight-shoota straight-shoota changed the title Change HTTP::Server::Response#respond_with_error to close the response Replace HTTP::Server::Response#respond_with_error with #respond_with_status Aug 3, 2019
@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch 2 times, most recently from 61206cb to 4c01851 Aug 3, 2019
@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from 4c01851 to b84256d Aug 4, 2019
@RX14
RX14 approved these changes Aug 10, 2019
@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from b84256d to da7820e Aug 10, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

Rebased and squashed. Also added the last commit which uses #respond_with_status in more places.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from da7820e to cbb46e4 Aug 13, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Rebased again on master with #8032 to see test_linux32 succeed.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-response-error-fixed branch from cbb46e4 to c54586f Aug 13, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

... aaand rebased again on master with #8056

@RX14
RX14 approved these changes Aug 27, 2019
@RX14 RX14 merged commit 194fcf4 into crystal-lang:master Aug 27, 2019
5 checks passed
5 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/http-response-error-fixed branch Sep 3, 2019
@bcardiff bcardiff added this to the 0.31.0 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.