Thanks @marshall007!

I think the API looks good.

@deontologician and @Tryneus could you review the JS and Python implementations respectively when you have a chance please?

One thing we should test is if these methods still work with SSL connections. They wouldn't be very useful at the moment, since whatever acts as an SSL proxy between the clients and the server is going to have a different port for its own outgoing connection to the server (this will change when we integrate SSL directly into the server). However we should make sure that they still return something reasonable and don't throw an error.
Until we have #4084, this might be a bit of a hassle to set up. I could probably launch a test instance on Compose and test it against that...

JS looks good 👍

Looks good, although it won't work for the Python async backends (asyncio, Tornado, and Twisted).

In the default net.py, ConnectionInstance has a member _socket corresponding to a SocketWrapper, but the behaviors are different for the other backends:

• net_asyncio.py: ConnectionInstance has a member _streamwriter, and you can get the underlying socket via _streamwriter.get_extra_info('socket')
• net_twisted.py: ConnectionInstance has a member _connection, which is a DatabaseProtocol object, and you can get the local connection info via _connection.transport.getHost(), I believe.
• net_tornado.py: ConnectionInstance has a member _socket, of the type socket.socket.

Because these behaviors are so different on each ConnectionInstance implementation, the ConnectionInstance itself should provide methods for reading these properties.

@marshall007, I understand if you don't want to spend the resources to implement and test this on the other Python backends, and if you prefer I can open an issue and add the other implementations myself once this PR has been merged.

@Tryneus thanks, that seems pretty straightforward. I'll update this PR to include as much of that as I can and let you know if I don't get around to some of it.

Also, can someone point me to the Ruby connection tests if they exist? I don't see any under rql_test/connections.

There currently isn't a direct Ruby equivalent of the connection tests for the other drivers. There are however a few tests for the EventMachine interface in test/rql_test(such asem.rb`). You could add a similar test for your client port function.

@Tryneus wasn't too bad after all. I added support for this to the remaining connection types with tests for each. It's ready for another look whenever you get the chance.

Awesome, looks good, @marshall007!

@danielmewes I added support for the Ruby driver with a test under connections/client_info.rb. Is that pretty much what you were saying I should do?

Aside from setting up explicit tests over SSL connections, I think that wraps it up.

Nice, I'll try to get the SSL testing done today or tomorrow.

@marshall007 the test is about what I had in mind. Your Ruby changes don't seem to be in this PR yet btw.

Oh wait, never mind. They're there. I must have been looking at an old version of the page.

@danielmewes haha, I just saw the same thing and was also confused.

Just tested all drivers with an SSL connection, and the client_port/client_address methods all work fine (except for the general problem with client_address in the Ruby implementation).

@danielmewes by the way, is it possible that the hostname we get back from all these different socket implementations could differ from the client_address in the jobs table? For example, does the server resolve hostnames to a numeric address or anything like that?

@marshall007 The server will always show numeric addresses I believe. It looks like the driver implementations (at least JS and Python) do the same, don't they?

However any NAT router or other higher-level proxy will of course cause mismatches in those addresses.
I'm not sure, but it's also possible that IPv4 addresses will be represented by an IPv6 address in some contexts.

That's one of the reasons for why an explicit client_id for the jobs table would be a lot nicer medium term...

@danielmewes I think in Node and Ruby we'll always get a numeric address back. The Python docs, however, seem to imply we could get back a hostname or address:

A pair (host, port) is used for the AF_INET address family, where host is a string representing either a hostname in Internet domain notation like daring.cwi.nl or an IPv4 address like 100.50.200.5, and port is an integer.

I don't think we need to worry about this right now, do you? You only really need client_port to filter the jobs table and client_id will be the more robust option once it's implemented anyway.

@marshall007 I agree that we shouldn't worry about it too much, since as you say the client_id will solve the issue better relatively soon.
If there are multiple client servers, the client_port is not going to be sufficient actually, since different client servers can assign the same client port to one of their connections.

In my brief test the Python driver actually did give me a numeric address though. I'm not sure under which circumstances it will report a hostname instead.
It sounds like socket.getaddrinfo is the way to get a numeric address from an arbitrary hostname or address string: https://docs.python.org/2/library/socket.html#socket.getaddrinfo
Do you think it would make sense to call that as part of the client_address() implementation for the Python driver, to make sure that we resolve any hostnames that might appear before returning the address to the user?

@danielmewes oh yea, I wasn't thinking about connecting to remote instances.

After looking into it more I think you only get back a hostname in Python if you construct the socket instance with a hostname and DNS resolution fails. If that is indeed true then I think we're fine not doing anything.

Self-assigning for making a final check, so we can merge this on time before 2.3.

Merged

This was before the Java driver, so there should probably be a corresponding change there

@deontologician Good call. Opened #5571