Issue #5020 Make servlets,filters,listeners beans again #5028
Conversation
Signed-off-by: Jan Bartel <[email protected]>
Signed-off-by: Jan Bartel <[email protected]>
There was a problem hiding this comment.
The new implementation of dumping with on-the-fly objects put into lists but then filtered out seems a nasty hack.
I'm not sure what the problem was and how this PR is supposed to fix it.
Can we get a dump before and after the changes so that we can reason about this change?
| static void dumpContainer(Appendable out, String indent, Container object, boolean last) throws IOException | ||
| { | ||
| dumpContainer(out, indent, object, (last ? Collections.emptyList() : Collections.singletonList(new Object()))); |
There was a problem hiding this comment.
Why are we dumping this new Object() created on the fly?
At least it deserves a comment.
There was a problem hiding this comment.
We are actually not dumping this object at all. If you look at the implementation of dumpContainer, it will skip any bean children of the Container that match any of the objects passed in as the list of extras. In this case, we are passing in either the empty list, or a singleton object as extras, neither of which will ever match anything. I've done this simply to maintain for backwards compatibility the old method signature of dumpContainer(Appendable, String, Container, boolean) - the fact of the matter is that we do not have any code anymore that calls that old method signature, so this (now deprecated) method can be entirely removed in jetty-10.
|
@sbordet as the issue points out, this change is nothing to do with changing the format of dump at all: it is actually to preserve the current dump output when we re-add servlets, filters and listeners as bean children of the ServletHandler, which has its own, very specific implemenation of dump() to order the dump output of those servlets, filters and listeners in a nice, grouped way. The generic dump() implementation merely outputs bean children in an ungrouped way: servlets, filters, listeners, filtermappings, servletmappings are all interleaved and hard to see. So, making servlets, filters and listeners again bean children of ServletHandler necessitated the ability of the generic dump mechanisms to avoid dumping certain beans, whose output is handled by ServletHandler.dump(), and Dumpable.dumpObjects. |
Signed-off-by: Jan Bartel <[email protected]>
| static void dumpContainer(Appendable out, String indent, Container object, boolean last) throws IOException | ||
| { | ||
| Container container = object; | ||
| ContainerLifeCycle containerLifeCycle = container instanceof ContainerLifeCycle ? (ContainerLifeCycle)container : null; | ||
| for (Iterator<Object> i = container.getBeans().iterator(); i.hasNext(); ) | ||
| { | ||
| Object bean = i.next(); | ||
|
|
||
| if (container instanceof DumpableContainer && !((DumpableContainer)container).isDumpable(bean)) |
There was a problem hiding this comment.
OK, This is a actually a much better way to accomplish this.
There was a problem hiding this comment.
Can dumpContainer be moved to be a static method of DumpableContainer to keep those concepts together?
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
Outdated
Show resolved
Hide resolved
| static void dumpContainer(Appendable out, String indent, Container object, boolean last) throws IOException | ||
| { | ||
| Container container = object; | ||
| ContainerLifeCycle containerLifeCycle = container instanceof ContainerLifeCycle ? (ContainerLifeCycle)container : null; | ||
| for (Iterator<Object> i = container.getBeans().iterator(); i.hasNext(); ) | ||
| { | ||
| Object bean = i.next(); | ||
|
|
||
| if (container instanceof DumpableContainer && !((DumpableContainer)container).isDumpable(bean)) |
There was a problem hiding this comment.
Can dumpContainer be moved to be a static method of DumpableContainer to keep those concepts together?
…andler.java Co-authored-by: Greg Wilkins <[email protected]>
jetty-util/src/main/java/org/eclipse/jetty/util/component/DumpableContainer.java
Outdated
Show resolved
Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/DumpableContainer.java
Outdated
Show resolved
Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/DumpableContainer.java
Outdated
Show resolved
Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/DumpableContainer.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jan Bartel <[email protected]>
joakime
deleted the
jetty-9.4.x-5020-MakeFiltersServletsListenersBeans
branch
Closes #5020
With commit 9d37feb servlets, filters and listeners were removed as bean children of the ServletHandler. We think this was done to improve the output of dump() so that the servlets, filters and listeners would all be appropriately grouped together, rather than randomly listed. Having removed them as beans, a Container.Listener no longer receives addBean() events for them, and thus libraries apparently like Guice which were relying on these events to add further LifeCycle.Listeners were impacted.
This PR is to restore the servlets, filters and listeners as bean children of the ServletHandler, but ensuring that their special servlet-spec mandated lifecycle is respected whilst also retaining the special formatting in dump().