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

Reimplement AttributeMap #3002

Closed
gregw opened this issue Oct 19, 2018 · 8 comments
Closed

Reimplement AttributeMap #3002

gregw opened this issue Oct 19, 2018 · 8 comments
Labels
Enhancement Stale For auto-closed stale issues and pull requests

Comments

@gregw
Copy link
Contributor

gregw commented Oct 19, 2018

AttributeMap needs to be reimplemented.

The AttributeMap contains a lazily created ConcurrentHashMap, but it is unclear if it necessary to avoid the creation lazily?

Also the implementation of Server.setAttribute is a crude attempt to allow JMX MBean creation for attribute values. It would be better if there was an option/version of AttributeMap that implemented the Container contract for it's values so that they could be listened for by the MBeanContainer.

@sbordet
Copy link
Contributor

sbordet commented Oct 19, 2018

ConcurrentHashMap used to be very expensive in terms of memory to allocate, and was the main reason behind AttributeMap allocating it lazily.

AttributeMap was the super class of many other classes allocated per-request, and that never used attributes, so it made sense allocate it lazily - it was never created.

@joakime
Copy link
Contributor

joakime commented Oct 19, 2018

So I asked myself ... Is is possible, with jetty-9.4.x to have an empty set of Attributes?

Based on experience, the Attributes map always seems to have something Jetty specific in it, So I decided to double-check.

The results of AttributesDump.java ...

2018-10-19 06:34:37.446:INFO::main: Logging initialized @327ms to org.eclipse.jetty.util.log.StdErrLog
2018-10-19 06:34:37.547:INFO:oejs.Server:main: jetty-9.4.12.v20180830; built: 2018-08-30T13:59:14.071Z; git: 27208684755d94a92186989f695db2d7b21ebc51; jvm 9.0.4+11
[context.attrs (post-initialized)] count = 2
[context.attrs (post-initialized)] [org.eclipse.jetty.util.DecoratedObjectFactory] = (org.eclipse.jetty.util.DecoratedObjectFactory)org.eclipse.jetty.util.DecoratedObjectFactory[decorators=1]
[context.attrs (post-initialized)] [org.eclipse.jetty.server.Executor] = (org.eclipse.jetty.util.thread.QueuedThreadPool)QueuedThreadPool[qtp1392425346]@52feb982{STARTED,8<=8<=200,i=8,q=0}[ReservedThreadExecutor@527e5409{s=0/8,p=0}]
2018-10-19 06:34:37.613:INFO:oejsh.ContextHandler:main: Started o.e.j.s.ServletContextHandler@27c86f2d{/,null,AVAILABLE}
[server.attrs (post-start)] count = 0
2018-10-19 06:34:37.779:INFO:oejs.AbstractConnector:main: Started ServerConnector@6a28ffa4{HTTP/1.1,[http/1.1]}{0.0.0.0:9090}
2018-10-19 06:34:37.780:INFO:oejs.Server:main: Started @668ms
[context.attrs (doGet)] count = 2
[context.attrs (doGet)] [org.eclipse.jetty.util.DecoratedObjectFactory] = (org.eclipse.jetty.util.DecoratedObjectFactory)org.eclipse.jetty.util.DecoratedObjectFactory[decorators=1]
[context.attrs (doGet)] [org.eclipse.jetty.server.Executor] = (org.eclipse.jetty.util.thread.QueuedThreadPool)QueuedThreadPool[qtp1392425346]@52feb982{STARTED,8<=8<=200,i=1,q=0}[ReservedThreadExecutor@527e5409{s=1/8,p=0}]
[request.attrs (doGet)] count = 0
Http.responseCode = 200
2018-10-19 06:34:37.923:INFO:oejs.AbstractConnector:main: Stopped ServerConnector@6a28ffa4{HTTP/1.1,[http/1.1]}{0.0.0.0:9090}
2018-10-19 06:34:37.925:INFO:oejsh.ContextHandler:main: Stopped o.e.j.s.ServletContextHandler@27c86f2d{/,null,UNAVAILABLE}

Result: of the default attributes found in the Server, ServletContext, and Request, only the ServletContext is always populated.

Note: the above test doesn't care about the "magic" attribute names that can be used on the API but don't have an associated AttributeMap entry.
Could we eliminate the default ServletContext attribute map entries for "magic" attribute names instead?

@gregw
Copy link
Contributor Author

gregw commented Oct 19, 2018

I think having the capability to have arbitrary attributes is valuable, so I don't think a "magic" only approach is good.

If we think there is still a need to lazily create the underlying map, then we can keep doing that, but we should review the class is doing that correctly.

My main concern is still the hack that we add values as beans in Server.setAttribute, I'm assuming so they can have JMX MBeans. This is strange and doesn't appear to respect deletions and replacements. An AttributeMap that correctly optionally respected the Container contract would be a better approach.

@sbordet
Copy link
Contributor

sbordet commented Oct 22, 2018

+1 on keeping lazy creation of the underlying map.

@stale
Copy link

stale bot commented Nov 20, 2019

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

@stale stale bot added the Stale For auto-closed stale issues and pull requests label Nov 20, 2019
@joakime
Copy link
Contributor

joakime commented Nov 20, 2019

Some observations of the current code in jetty-9.4.x.

A standard WebAppContext, with a single servlet added.

I set a breakpoint on the doGet() i see the following AttributeMap state on a default server configuration.

  1. Request attributes is null
  2. ServletContextHandler$Context attributes has 1 entry.
  • org.eclipse.jetty.util.DecoratedObjectFactory -> {DecoratedObjectFactory@2106} "org.eclipse.jetty.util.DecoratedObjectFactory[decorators=1]"
  1. ContextHandler attributes has 1 entry.
  • org.eclipse.jetty.server.Executor -> {QueuedThreadPool@2085} "QueuedThreadPool[qtp982757413]@3a93b025{STARTED,8<=8<=200,i=3,r=20,q=0}[ReservedThreadExecutor@4977bacd{s=0/20,p=0}]"

Perhaps we need to review why we have so many places for attributes as well?

@stale stale bot removed the Stale For auto-closed stale issues and pull requests label Nov 20, 2019
@stale
Copy link

stale bot commented Nov 25, 2020

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.

@stale stale bot added the Stale For auto-closed stale issues and pull requests label Nov 25, 2020
@gregw
Copy link
Contributor Author

gregw commented Nov 25, 2020

Already done in #4816

@gregw gregw closed this as completed Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Stale For auto-closed stale issues and pull requests
Projects
None yet
Development

No branches or pull requests

3 participants