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

Freeze HttpFields #10339

Merged
merged 18 commits into from
Aug 25, 2023
Merged

Freeze HttpFields #10339

merged 18 commits into from
Aug 25, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Aug 18, 2023 •
edited
Loading

Provide a mechanism to freeze a HTTP response headers so that container provided headers are not lost in a reset. From question in #10310

@gregw gregw self-assigned this Aug 18, 2023
@gregw gregw marked this pull request as ready for review August 18, 2023 01:31
@gregw gregw requested review from joakime and sbordet August 18, 2023 01:31
@gregw
Copy link
Contributor Author

gregw commented Aug 18, 2023

Currently I have explicit attempts to remove frozen fields throwing ISE. Other options include:

  1. throw UnsupportedOperationException
  2. don't throw and silently ignore
  3. allow explicit removal and only protect from a clear()

I think I'm leaning to 2) else 1) but maybe 3)... no strong feelings. Thoughts?

This was referenced Aug 18, 2023
Merged
Merged
@gregw
Copy link
Contributor Author

gregw commented Aug 20, 2023

@sbordet I've implemented this with a specific field type like you suggested.

@joakime joakime mentioned this pull request Aug 21, 2023
Merged
@joakime
Copy link
Contributor

joakime commented Aug 21, 2023

The changes from PR #10310 have been merged, lets merge those into this PR and continue the reset behaviors here.

@gregw
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
30d6f60 
# Conflicts:
#	jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
#	jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java
@gregw
Copy link
Contributor Author

gregw commented Aug 21, 2023

@joakime @sbordet review please before it goes stale again!

joakime
joakime reviewed Aug 22, 2023
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Do we have a testcase where the Server and/or X-Powered-By headers are set by a handler to something different?

What about apps that want to set the Date header? can they?

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated
@@ -1185,7 +1184,7 @@ public void setStatus(int code)
}

@Override
public HttpFields.Mutable getHeaders()
public ResponseHttpFields getHeaders()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need to be changed?
I cannot see any unique methods that only ResponseHttpFields implement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for the previous implementation that did freeze/thaw. It is no longer needed so I will revert.

@gregw
Copy link
Contributor Author

gregw commented Aug 22, 2023

@joakime

Do we have a testcase where the Server and/or X-Powered-By headers are set by a handler to something different?

Not explicitly, but it is the same case as the Date header below

What about apps that want to set the Date header? can they?

We do have tests that explicitly set the Date header and the ResponseTest also checks that persistent headers can be modified with iterator sets. The original change treated these fields as immutable, which was incorrect and I changed to make sure only that they cannot be deleted, but they can be modified (if the HttpFields itself is in a mutable state).

I'll add a test for a put of SERVER.

janbartel
janbartel previously approved these changes Aug 23, 2023
View reviewed changes
lorban
lorban previously requested changes Aug 23, 2023
View reviewed changes
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java
i.remove();
_current = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also throw ISE when _current is null according to the Iterator.remove() javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The i.remove() on the previous line will do that. But I will add an explicit check

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java
}

@Override
public void remove()
{
if (_committed.get())
throw new UnsupportedOperationException("Read Only");
if (isPersistent(_current))
throw new IllegalStateException("Persistent field");
Copy link
Contributor

Choose a reason for hiding this comment

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

My interpretation of the Iterator.remove() javadoc makes me think UnsupportedOperationException should be thrown here.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java
i.remove();
_current = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also throw ISE when _current is null according to the Iterator.remove() javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the i.remove() call above will do that. I can explicitly do the check if you really want ?

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java Show resolved Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java Show resolved Hide resolved
sbordet
sbordet previously requested changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

See also my comments about this PR in #10340.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java Show resolved Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java Show resolved Hide resolved
@joakime joakime added this to the 12.0.x milestone Aug 23, 2023
joakime
joakime reviewed Aug 24, 2023
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

I'd like to add a testcase all the way up in ee10-servlets to verify the behavior up there too.
Mind if I add it to this branch?

@gregw gregw dismissed stale reviews from sbordet and lorban August 25, 2023 03:01

was approved in #10340 and only nit fixed afterwards

@gregw gregw merged commit 4086c7e into jetty-12.0.x Aug 25, 2023
2 checks passed
@gregw gregw deleted the fix/10310/freezeHttpFields branch August 25, 2023 03:02
gregw added a commit that referenced this pull request Aug 28, 2023
@gregw
Fix #10416 EE9 Response headers
db6acdd 
Use the core response HttpFields directly as the ee9 response headers to avoid copy and retain persistent field behaviour.

Added EE9 test to show that Persistent fields can be modified

Updated fix for #10339 so that persistent fields revert to original values after a clear operation
@gregw gregw mentioned this pull request Aug 28, 2023
Merged
gregw added a commit that referenced this pull request Aug 28, 2023
@gregw
Fix #10416 Do not copy ee9 response headers (#10417)
e3ee84a 
Use the core response HttpFields directly as the ee9 response headers to avoid copy and retain persistent field behaviour.
Fix #10416 EE9 Response headers
Added EE9 test to show that Persistent fields can be modified
Updated fix for #10339 so that persistent fields revert to original values after a clear operation
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

@olamy olamy olamy approved these changes

@sbordet sbordet Awaiting requested review from sbordet

@janbartel janbartel Awaiting requested review from janbartel

@lorban lorban Awaiting requested review from lorban

Assignees

@gregw gregw

Labels
None yet
Projects
None yet
Milestone
12.0.x
Development

Successfully merging this pull request may close these issues.
6 participants
@gregw @joakime @olamy @janbartel @sbordet @lorban