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.

Sign up for GitHub

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

Jump to bottom

Improve Attributes Handling #4816

Merged
merged 5 commits into from
Apr 28, 2020
Merged

Improve Attributes Handling #4816

merged 5 commits into from
Apr 28, 2020

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Apr 27, 2020

Spun out from #4814

Improve attribute handling to reduce garbage and improve lookup.
Introduced a Wrapper so that request can remove any layers on reset.

Signed-off-by: Greg Wilkins gregw@webtide.com

@gregw
Spun out from #4814 Improve Attributes Handling
862562c 
Improve attribute handling to reduce garbage and improve lookup.
Introduced a Wrapper so that request can remove any layers on reset.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw mentioned this pull request Apr 27, 2020
Merged
gregw added a commit that referenced this pull request Apr 27, 2020
@gregw
Issue #4814 Configuring Connection Factory
57a02bd 
Redo of this PR without Attributes improvements (moved to #4816).
Add a ConnectionFactory.Configuring interface to all connectors to be configured during doStart.
I have some concern about shared HttpConfigurations.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@joakime
Issue #4814 - Exposing AttributeMap.getAttributeNameSet() on Attributes.
d460786 
The underlying AttributesMap already has a .getAttributeNameSet()
method, expose it on the Attributes interface.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime
joakime reviewed Apr 27, 2020
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added commit d460786 to this PR to expose the AttributesMap.getAttributeNameSet() on the Attributes interface and use it.

Feel free to reject / rollback it if you don't like that change.

Since this PR is an effort to reduce GC, this change eliminates the intermediary Enumeration on some code paths.

jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java Outdated Show resolved Hide resolved
@gregw
Copy link
Contributor Author

gregw commented Apr 27, 2020

@joakime I like the Set access, but that method now also needs to be overridden in the wrapping Attributes... actually if done right then getAttributeNameSet needs to be overridden and getAttributeNames can use that. As it currently is, any body calling getAttributeNameSet will not see the extra names on a forward or include. Can you fix?

@joakime
Copy link
Contributor

joakime commented Apr 27, 2020

@joakime I like the Set access, but that method now also needs to be overridden in the wrapping Attributes... actually if done right then getAttributeNameSet needs to be overridden and getAttributeNames can use that. As it currently is, any body calling getAttributeNameSet will not see the extra names on a forward or include. Can you fix?

Yup, I can take care of that.

gregw and others added 3 commits April 27, 2020 15:32
@gregw
Allow a set to override a secure attribute.
2d7b299 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@joakime
Issue #4814 - Attributes.getAttributeNames() is now defaulted
663ef83 
The Attributes.getAttributeNames() will use the
.getAttributeNameSet() by default now.

Updated all Attributes.Wrapper impls to use this new behavior

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Merge branch 'jetty-9.4.x-4814-ImproveAttributeHandling' of github.co…
e487b16 
…m:eclipse/jetty.project into jetty-9.4.x-4814-ImproveAttributeHandling
@joakime
Copy link
Contributor

joakime commented Apr 27, 2020

@gregw the change you asked for to impl Set in wrapper can be found in commit 663ef83

@gregw
Copy link
Contributor Author

gregw commented Apr 28, 2020

I think no matter what, if one of those attributes is set we should return that value.

@gregw gregw merged commit 8fcbf6d into jetty-9.4.x Apr 28, 2020
@gregw gregw deleted the jetty-9.4.x-4814-ImproveAttributeHandling branch April 28, 2020 08:57
gregw added a commit that referenced this pull request Apr 29, 2020
@gregw
Jetty 10.0.x 4814 configuring connection factory (#4815)
81c4663 
* Issue #4814 Configuring Connection Factory

Redo of this PR without Attributes improvements (moved to #4816).
Add a ConnectionFactory.Configuring interface to all connectors to be configured during doStart.
I have some concern about shared HttpConfigurations.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* updates from review

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@joakime joakime mentioned this pull request May 29, 2020
Closed
@lachlan-roberts lachlan-roberts mentioned this pull request Jun 25, 2020
Closed
@gregw gregw mentioned this pull request Nov 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime left review comments

@lachlan-roberts lachlan-roberts lachlan-roberts approved these changes

@lorban lorban Awaiting requested review from lorban

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gregw @joakime @lachlan-roberts