Change HTTP::Request#remote_address type to Socket::Address?

[waj]

This is a breaking change, but probably with a very low impact.

Currently the remote_address is a String with the format "ip:port", so if just the IP is needed extra parsing must be done. Also, this is consistent with other occurrences of local_address and remote_address in the std.

[asterite]

I implemented it as a string because in Go it's a string. I'm sure there must be a good reason why the chose String. Why do you need the address?

[waj]

In this case for logging just the IP. The port changes on every request and is not useful at all for 99.99% of the cases.

Other useful cases are auditing, filtering, geolocation of clients, stats. All these cases need just the IP address without the port.

[asterite]

Can you split the string by : and just keep the left-hand side?

I think this PR might be fine, but I'd like to know why Go chose to use a String.

[straight-shoota]

I believe a string is just more flexible.

The port changes on every request and is not useful at all for 99.99% of the cases.

In light of that we might perhaps consider only storing the IP address in remote_address. That would need to be a string because we have no data type for only the IP without port.

[waj]

Go is not the only language out there 🙂

• Ruby: Rack has a request.ip.
• Dart: https://api.dart.dev/stable/2.8.1/dart-io/HttpConnectionInfo-class.html
• .NET: RemoteIpAddress

[waj]

I believe a string is just more flexible

Of course. What's more flexible than a string? But we don't put strings everywhere for that reason. And the address can be easily converted to a string.

[waj]

Can you split the string by : and just keep the left-hand side?

Of course I can parse the value, but first it has to be matched with a regex in case is not an IPAddress. But why add that overhead when the original IPAddress was already known?

In the future, if we find another possibility for Address that doesn't fit within the concept of a Socket we could move that class to a different, more general module and add another subclass.

[waj]

FYI, I just tested the performance of samples/http_server.cr in my machine with wrk and I obtain 130k req/s without the change and 139k req/s with it. Not a big difference specially thinking that this "hello world" app is not representative but to see that there is an impact creating extra strings.

[asterite]

Cool! If other languages provide a non-string value then this change is perfect 💯

[waj]

I just disabled some specs in win32, mostly for HTTP. I'm confused about the require "http" within src/mime/media_type.cr but I removed it and then I cannot run some individual specs. So probably a task for a separate PR.

[straight-shoota]

Disabling that many specs which worked before this PR is not a good idea. We can just make remote_address = nil on win32 for now and it should be fine.

[waj]

Ok, I’ll try that

[waj]

@straight-shoota the specs in win32 are back. I left String? as the type for remote_address in Windows for now.

[straight-shoota]

Why String? Can't it just be nil on win32?

Public APIs should not have different types depending on the platform.

[waj]

I put String to avoid disabling more specs. Nobody is going to use HTTP on Windows yet, because sockets are not working yet, AFAIK.

I mean, I don't expect the APIs to be different once it's working on Windows.