Repository navigation
Also expose port for each connection #26
Description
Currently, the Connection::getRemoteAddress() method only returns the IP address of the remote peer.
There's currently no way to access its TCP/IP port.
It's quite trivial to add a getRemotePort() method, but we should think twice about whether we should limit ourselves to TCP/IP here.
Other schemes (such as Unix or perhaps even raw sockets) do not have a concept of "ports" and may not necessarily build on top of IP.
As such, we should consider just returning the full remote peer address from the getRemoteAddress() method.
Activity
clue commentedon Apr 20, 2016
See also reactphp/reactphp#199
SamMousa commentedon Jul 12, 2016
Alternatively it would be great to at least not remove it from the address.
Are there any plans to add this to the interface at some point?
To properly support proxying one would want to be able to override this function to specify the port / address of the other end of the tunnel... In such a scenario manually calling
stream_get_name()does not suffice.I'm not sure what the downside is of adding a getter for the port that returns
nullwhen there is no port (ie in case of a Unix socket). Alternatively we could create aTcpConnectionInterfacethat has a port.Is any development / discussion on this topic being held elsewhere?
clue commentedon Jul 12, 2016
This ticket has received little interest so far. Care to change this an file a PR? :)
I think this is what makes most sense, see also reactphp/reactphp#199 for more details.
SamMousa commentedon Jul 12, 2016
@clue I've read reactphp/reactphp#199, the issue is that the longer we wait with making a choice the more libraries will break when we do decide to change it.
Just returning the result from
stream_get_nameis in my opinion not the best thing to do.Of course it could be interesting to formalize addresses as well. (
AddressInterface) but that might be overkill.I'm definitely available to make a PR but let's first agree on the desired direction.
https://github.com/SAM-IT/react-socket
clue commentedon Jul 12, 2016
Care to elaborate what your opinion is based on?
I agree that this would be the cleanest and totally overkill solution :)
SamMousa commentedon Jul 12, 2016
Well the whole point of implementing a
Connectionis abstraction. Returning a string like that doesn't feel right to me.clue commentedon Jul 12, 2016
For the most part,
Connectionis an abstraction of a duplex stream (plus a single accessor method for the remote address). Given the little interest in this ticket so far and looking at existing libraries building on top of this library, the remote address appears to used way less than the stream abstraction (from what I can tell).Technically, the "port" is part of the "address" (for TCP/IP anyway).
We could introduce a second accessor method for the remote port and then rename the first method to make it obvious it does not include the port. However, this would be a leaky abstraction as we've now leaked the detail that we focus on TCP/IP connections and other protocols do not know the concept of a "port".
Alternatively, we could change the single accessor method to return the full address. The string format returned by the underlying
stream_get_name()would probably be the simplest solution here. This is in line with how URIs are represented throughout React's components (or planned for the future as per reactphp/reactphp#199) and many other third-party libraries.I agree that having an interface (kind of like PSR-7's
UriInterface) sounds tempting here. If we want to implement this throughout React's components we'd have to add dependencies to a shared component. For consistency, we'd likely also type-hint against this and thus require users to create new instances before establishing connections to URIs or setting up listening addresses.As such, I'm in favor of using a single method to access the URI and I'm not really convinced an additional interface is actually worth the hassle. I'm curious what others think about this?
SamMousa commentedon Jul 12, 2016
That sounds ok to me, and I agree that doing a true abstraction requires too much overhead and a leaky abstraction is not what we want; so that leaves
getRemoteAddress.