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

Expose TCP connection establishment information #9529

Open
sbordet opened this issue Mar 23, 2023 · 9 comments · May be fixed by #12570
Open

Expose TCP connection establishment information #9529

sbordet opened this issue Mar 23, 2023 · 9 comments · May be fixed by #12570
Assignees
Labels
Enhancement Help Wanted Pinned Issues that never go stale

Comments

@sbordet
Copy link
Contributor

sbordet commented Mar 23, 2023

Jetty version(s)
10+

Enhancement Description
Would be nice to have HttpClient (or some other component such as ClientConnector) emit events for TCP connection establishment/failure (a begin event, and a success/fail event, for a given SocketAddress).

This could be useful to track destination servers that are faulty or take a long time to establish a connection.

Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Mar 23, 2024
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label Mar 24, 2024
@sbordet sbordet self-assigned this Aug 28, 2024
@sbordet sbordet added the Pinned Issues that never go stale label Sep 3, 2024
@sbordet sbordet removed this from Jetty 12.0.14 Sep 25, 2024
@arsenalzp
Copy link
Contributor

arsenalzp commented Oct 16, 2024

Hello,
It is me again.
As far as I understand you want to have events emitter/listener for TCP conn established/failed to establish in additional to onOpen/onClose events. Am I right?
How complex do you think this task is? Can I work on it?

@sbordet
Copy link
Contributor Author

sbordet commented Oct 16, 2024

@arsenalzp I don't think it's too complex, so you can give it a go.

You can start by finding the place in the code that initiate connection establishment, the place where the connection establishment is complete, and the place where the connection establishment is failed.

Then it's a matter of designing the listener APIs, and then emitting the events.

Let me know if you need guidance, but please one at a time 😄

Thanks!

@arsenalzp
Copy link
Contributor

arsenalzp commented Oct 17, 2024

I guess the right place is in ClientConnector.ClientSelectorManager

        public void connectionOpened(Connection connection, Object context)
        {
          ....
        }
        protected void connectionFailed(SelectableChannel channel, Throwable failure, Object attachment)
        {
         ....
        }

A draft implementation is showing connectionFailed() method is invoked more than one time, I suppose it is due to try-reconnect behavior. Is it valid to invoke onConnectionFailed() listener method more than one time?
Another question - what is listener methods signature, I mean what should be put as argument:

void onConnectionEstablished(Connection c)
void onConnectionFailed(Throwable failure)

@sbordet
Copy link
Contributor Author

sbordet commented Oct 17, 2024

@arsenalzp connectionFailed() should not be called multiple times.
Do you have stack traces from where it is called the first time, and the subsequent times?

We want to track connection establishment, so we want an event when the connection is initiated, and one when it is completed (either success or failure).
See ClientConnector.connect() as the initiating point.

The API should be lower level, so:

onConnectBegin(SocketChannel s)
onConnectSuccess(SocketChannel s);
onConnectFailure(SocketChannel s, Throwable x);

@arsenalzp
Copy link
Contributor

arsenalzp commented Oct 18, 2024

I found a connection behaviour, localhost host name is resolved into two IP-addresses: IPv4 and IPv6, so HttpClient tries to connect to both addresses. If host name is resolved in more addresses (addresses number are getting from socketAddresses.size()) then HttpClient tries to connect to all of them:

private void connect(List<InetSocketAddress> socketAddresses, int index, Map<String, Object> context)
 {
                    context.put(HttpClientTransport.HTTP_CONNECTION_PROMISE_CONTEXT_KEY, new Promise.Wrapper<>(promise)
                    {
                        @Override
                        public void failed(Throwable x)
                        {
                            int nextIndex = index + 1;
                            if (nextIndex == socketAddresses.size())
                                super.failed(x);
                            else
                                connect(socketAddresses, nextIndex, context);
                        }
                    });
                    HttpClient.this.transport.connect((SocketAddress)socketAddresses.get(index), context);
}

@sbordet
Copy link
Contributor Author

sbordet commented Oct 21, 2024

@arsenalzp good finding!

Can you please check that in the "begin" event, you get two different SocketChannel instances?
Can you also check that in the "begin" event, you can access the SocketAddress.getLocalAddress() and it reports in one case the IPv4 and in the other the IPv6?

We should get 2 "begin" events and 2 "failure" events and 0 "success" events.
If that's the case, everything works as expected and you can wire in the listener to emit the events to.

@arsenalzp
Copy link
Contributor

arsenalzp commented Oct 22, 2024

@arsenalzp good finding!

Can you please check that in the "begin" event, you get two different SocketChannel instances? Can you also check that in the "begin" event, you can access the SocketAddress.getLocalAddress() and it reports in one case the IPv4 and in the other the IPv6?

We should get 2 "begin" events and 2 "failure" events and 0 "success" events. If that's the case, everything works as expected and you can wire in the listener to emit the events to.

Yes, of course, there two different SocketChannel instances.

However, it is not possible to get local address when channel is not connected (connection state is pending), meanwhile it is possible to get remote address, check a channel state, etc.

Moreover, calling getLocalAddress on closed channel throws ClosedChannelException exception.

My expectation is that bind operation is the first one before any connection attempt.
Look at the ClientConnector.connect() it is good place for onConnectionBegin event.
However, there is no bind operation for SocketChannel instance, but NetworkChannel one, so it is not possible to get local address by calling getLocalAddress() method; doing so method returns null .

Obviously, connection can't be established without binding to local address and port, so this operation is done somewhere else (ManagedSelector.submit()).
Here is Javadoc for SocketChannel:

public abstract SocketChannel bind(SocketAddress local)
throws IOException

Binds the channel's socket to a local address.

This method is used to establish an association between the socket and a local address. Once an association is established then the socket remains bound until the channel is closed. If the local parameter has the value null
then the socket will be bound to an address that is assigned automatically.

It means getting null local address during the initial phase of connection is valid behavior.

It is still possible to get local address on successful connection event.

@arsenalzp
Copy link
Contributor

/assign

@sbordet sbordet linked a pull request Nov 25, 2024 that will close this issue
sbordet pushed a commit that referenced this issue Nov 25, 2024
Introduced ClientConnector.ConnectListener with events for TCP connection establishment/failure.

Signed-off-by: Oleksandr Krutko <[email protected]>
sbordet added a commit that referenced this issue Nov 25, 2024
Cleaned up contribution by @arsenalzp.
Added more test cases for blocking and non-blocking.

Signed-off-by: Oleksandr Krutko <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet removed this from Jetty 12.1.0 Nov 25, 2024
@sbordet sbordet moved this to 👀 In review in Jetty 12.0.16 Nov 25, 2024
@sbordet sbordet moved this from 👀 In review to 🏗 In progress in Jetty 12.0.16 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Help Wanted Pinned Issues that never go stale
Projects
Status: 🏗 In progress
3 participants