search

jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.86k stars 1.91k forks source link

Add ClientConnectionListener listener #12421

Open arsenalzp opened 3 weeks ago

arsenalzp commented 3 weeks ago

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.

gregw commented 3 weeks ago

@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 commented 3 weeks ago

@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!

arsenalzp commented 3 weeks ago

@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.

arsenalzp commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

@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 commented 2 weeks ago

@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.

arsenalzp commented 2 weeks ago

@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 commented 2 weeks ago

@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).

arsenalzp commented 2 weeks ago

@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.