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

Add connection metadata to HTTP::Server::Response #5870

Conversation

straight-shoota
Copy link
Member

This PR adds methods #local_address and #remote_address address to HTTP::Server::Response.

I'm not entirely sure whether the method's return type should be nilable. It's not very typical to have a Response not wrapping a socket. This is mostly used as mockup for specs. Maybe we can avoid forcing usually unnecessary nil-checks in actual code. This could be done either by raising (though there should probably also be a non-raising option) or returning some default address if the IO does not provide one.

Closes #5784
Based on #5869

@straight-shoota
Copy link
Member Author

I'm not sure anymore about whether this is a good solution. It's not ideal, but at least relatively easy compared to a bigger refactor (see #5784 (comment)).

@asterite
Copy link
Member

Yeah, maybe having separate Request types is the way to go.

@fuegito
Copy link

fuegito commented Jun 11, 2018

Hi all,
is there a chance that this PR will be merged any time soon? I just can't understand why this hasn't been added to the stdlib from the beginning. It is a no-brainer to have this feature since it is anything but an exotic requirement to know (e.g. for logging) the connecting ip.

Note that i am not interested in the (real) client ip (i know this is more complicated) but in the remote socket source ip and yes, i know the workarounds with adding an HTTP-Header through a reverse proxy.

BTW: how is this patch applied in the most canonical way (best practice) as long as it isn't merged?

Thanks for your reply & best regards,
Stefan

@straight-shoota
Copy link
Member Author

straight-shoota commented Jun 11, 2018

I don't think this should be merged soon. @asterite's suggestion to separate HTTP::Request seems the way to go IMHO.

You shouldn't apply this patch directly to your local std lib copy, but rather add a monkey patch in your application code:

class HTTP::Server::Response
  # Returns the local address of the network socket used by this connection.
  #
  # This can be useful if the server listens on multiple sockets.
  def local_address : Socket::Address?
    if (socket = @io).responds_to? :local_address
      socket.local_address
    end
  end
 
  # Returns the remote address of the network socket used by this connection.
  #
  # This is not necessarily the address of the original requestor but the last
  # proxy (e.g. a load balancer).
  def remote_address : Socket::Address?
    if (socket = @io).responds_to? :remote_address
      socket.remote_address
    end
  end
end

@straight-shoota
Copy link
Member Author

You probably won't need any of the changes in the first commit, they'll be in 0.25.0 (see #5869).

@fuegito
Copy link

fuegito commented Jun 11, 2018

Great, thanks a lot!

BTW: I am impressed by the quick response I received here!

@straight-shoota
Copy link
Member Author

I'll close this. Let's discuss a more general solution regarding HTTP::Request.

@straight-shoota straight-shoota deleted the jm/feature/http-server-response-metadata branch July 2, 2018 17:38
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