Add ClientConnectionListener listener #12421

arsenalzp · 2024-10-23T08:44:27Z

Here is the draft of changes proposed to solve #9529
Let's discuss it in the issue thread as the implementation was not hardened yet.

Signed-off-by: Oleksandr Krutko <[email protected]>

gregw · 2024-10-24T05:43:20Z

@arsenalzp Would it be OK if we implemented this in jetty-12.1.0 rather than 12.0.x? We are trying to feature freeze 12.0

arsenalzp · 2024-10-24T07:39:25Z

@arsenalzp Would it be OK if we implemented this in jetty-12.1.0 rather than 12.0.x? We are trying to feature freeze 12.0

This approach is OK for me!

sbordet

@arsenalzp good start.

Please review my comments, but I really need you to add test cases.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

arsenalzp · 2024-10-25T14:42:59Z

@arsenalzp good start.

Please review my comments, but I really need you to add test cases.

Of course it is good to have test cases, however as I said it is the draft, just to show the framework of solution.

Signed-off-by: Oleksandr Krutko <[email protected]>

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

arsenalzp · 2024-10-25T21:30:21Z

I'm thinking about the place of ConnectListenerTest test class.
Naturally it should be put inside org.eclipse.jetty.io package, in that case it is necessary to import Server and HttpClient classes and it leads to modifying of pom.xml, however it causes cyclic reference.

What do you think?

sbordet · 2024-10-25T21:49:53Z

@arsenalzp put the test class in the jetty-tests/jetty-test-client-transport module, and test it as parametrized test with transportsTCP (see examples in that module).

sbordet · 2024-10-25T21:51:26Z

@arsenalzp please also fix the formatting, see here why the build fails:
https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/detail/PR-12421/3/pipeline

arsenalzp · 2024-10-26T10:54:04Z

@arsenalzp please also fix the formatting, see here why the build fails: https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/detail/PR-12421/3/pipeline

Yeah, issue has been fixed already.

sbordet

@arsenalzp another thing.
Instead of calling getBeans(ClientConnector.ConnectListener.class) every time, which is an expensive operation, please override add/removeEventListener, and store the ConnectListener in a local CopyOnWriteArrayList field, so the notification is faster.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

arsenalzp · 2024-10-31T17:13:54Z

@arsenalzp another thing. Instead of calling getBeans(ClientConnector.ConnectListener.class) every time, which is an expensive operation, please override add/removeEventListener, and store the ConnectListener in a local CopyOnWriteArrayList field, so the notification is faster.

Hello,
What is the idea here?
It is already implemented by superclass ContainerLifeCycle:

private final List<Bean> _beans = new CopyOnWriteArrayList<>();
private final List<Container.Listener> _listeners = new CopyOnWriteArrayList<>();

sbordet · 2024-10-31T18:30:57Z

@arsenalzp in the base class you have a field for List<Container.Listener> so that they can be accessed efficiently without having to look up the beans all the times.

I am proposing that you do similarly in ClientConnector, keeping a field of List<ConnectListener> so that those listeners can be accessed very efficiently, instead of calling getBeans(ClientConnector.ConnectListener.class).

Signed-off-by: Oleksandr Krutko <[email protected]>

arsenalzp · 2024-10-31T19:43:11Z

@arsenalzp in the base class you have a field for List<Container.Listener> so that they can be accessed efficiently without having to look up the beans all the times.

I am proposing that you do similarly in ClientConnector, keeping a field of List<ConnectListener> so that those listeners can be accessed very efficiently, instead of calling getBeans(ClientConnector.ConnectListener.class).

It is clear for me now, thank you!
This improvement has just been implemented.

sbordet

@arsenalzp another good step, please review my comments.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

sbordet · 2024-11-03T18:00:52Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

                boolean blocking = isConnectBlocking() && address instanceof InetSocketAddress;
                if (LOG.isDebugEnabled())
                    LOG.debug("Connecting {} to {}", blocking ? "blocking" : "non-blocking", address);
                if (blocking)
                {
+                    listeners.forEach(listener -> listener.onConnectBegin(socketChannel, socketAddress));


These calls should be wrapped, since we do not want an exception thrown from onConnectBegin() to stop the connection.
Make a method, say connectBegin(), use a for loop instead of forEach(), and wrap listener.onConnectBegin() into try/catch(Throwable).
And same for the other events.

What is the reason of this solution?
ConnectListener methods don't throw an exception.

ConnectListener implementations are user code, and it can throw any RuntimeException, but we do not want ClientConnector internal working to be affected by that if it happens.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/SelectorManager.java

Signed-off-by: Oleksandr Krutko <[email protected]>

sbordet

@arsenalzp looks good, two things:

wrap the calls to ConnectListener methods into try/catch(Throwable).
Write tests.

Thanks!

sbordet · 2024-11-09T12:30:48Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

+    {
+        if (listener instanceof ConnectListener connectListener)
+            return listeners.add(connectListener);
+        return false;


No, you need to call super here.

sbordet · 2024-11-09T12:31:02Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

+    {
+        if (listener instanceof ConnectListener connectListener)
+            return listeners.remove(connectListener);
+        return false;


No, you need to call super here.

sbordet · 2024-11-09T12:31:20Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/SelectorManager.java

+     */
+    protected void connectSuccess(SelectableChannel channel)
+    {
+


Remove this newline.

sbordet · 2024-11-09T12:31:53Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java

@@ -753,4 +768,29 @@ public ChannelWithAddress newChannelWithAddress(ClientConnector clientConnector,
            };
        }
    }
+
+    public interface ConnectListener extends EventListener


Please add javadocs to class and methods.

add ClientConnectionListener listener

642189a

Signed-off-by: Oleksandr Krutko <[email protected]>

joakime requested a review from sbordet October 23, 2024 13:48

joakime marked this pull request as draft October 23, 2024 13:48

joakime added the Enhancement label Oct 23, 2024

sbordet requested changes Oct 25, 2024

View reviewed changes

improve naming conventions and methods signature for ConnectListener

3f8f31f

Signed-off-by: Oleksandr Krutko <[email protected]>

sbordet requested changes Oct 25, 2024

View reviewed changes

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Outdated Show resolved Hide resolved

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Outdated Show resolved Hide resolved

sbordet requested changes Oct 27, 2024

View reviewed changes

arsenalzp added 2 commits October 31, 2024 20:52

add SelectorManager.onConnected() callback

880327e

Signed-off-by: Oleksandr Krutko <[email protected]>

improve the listeners iteration

1277964

Signed-off-by: Oleksandr Krutko <[email protected]>

sbordet requested changes Nov 3, 2024

View reviewed changes

remove duplicate calls, improve stability

b2c2f4d

Signed-off-by: Oleksandr Krutko <[email protected]>

sbordet requested changes Nov 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClientConnectionListener listener #12421

Add ClientConnectionListener listener #12421

arsenalzp commented Oct 23, 2024 •

edited

Loading

gregw commented Oct 24, 2024

arsenalzp commented Oct 24, 2024

sbordet left a comment

arsenalzp commented Oct 25, 2024

arsenalzp commented Oct 25, 2024

sbordet commented Oct 25, 2024

sbordet commented Oct 25, 2024

arsenalzp commented Oct 26, 2024

sbordet left a comment

arsenalzp commented Oct 31, 2024 •

edited

Loading

sbordet commented Oct 31, 2024

arsenalzp commented Oct 31, 2024

sbordet left a comment

sbordet Nov 3, 2024

arsenalzp Nov 7, 2024

sbordet Nov 9, 2024

sbordet left a comment •

edited

Loading

sbordet Nov 9, 2024

sbordet Nov 9, 2024

sbordet Nov 9, 2024

sbordet Nov 9, 2024

Add ClientConnectionListener listener #12421

Are you sure you want to change the base?

Add ClientConnectionListener listener #12421

Conversation

arsenalzp commented Oct 23, 2024 • edited Loading

gregw commented Oct 24, 2024

arsenalzp commented Oct 24, 2024

sbordet left a comment

Choose a reason for hiding this comment

arsenalzp commented Oct 25, 2024

arsenalzp commented Oct 25, 2024

sbordet commented Oct 25, 2024

sbordet commented Oct 25, 2024

arsenalzp commented Oct 26, 2024

sbordet left a comment

Choose a reason for hiding this comment

arsenalzp commented Oct 31, 2024 • edited Loading

sbordet commented Oct 31, 2024

arsenalzp commented Oct 31, 2024

sbordet left a comment

Choose a reason for hiding this comment

sbordet Nov 3, 2024

Choose a reason for hiding this comment

arsenalzp Nov 7, 2024

Choose a reason for hiding this comment

sbordet Nov 9, 2024

Choose a reason for hiding this comment

sbordet left a comment • edited Loading

Choose a reason for hiding this comment

sbordet Nov 9, 2024

Choose a reason for hiding this comment

sbordet Nov 9, 2024

Choose a reason for hiding this comment

sbordet Nov 9, 2024

Choose a reason for hiding this comment

sbordet Nov 9, 2024

Choose a reason for hiding this comment

arsenalzp commented Oct 23, 2024 •

edited

Loading

arsenalzp commented Oct 31, 2024 •

edited

Loading

sbordet left a comment •

edited

Loading