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

Implement containsLast in HttpFields #10340

Merged
merged 44 commits into from
Aug 26, 2023
Merged

Implement containsLast in HttpFields #10340

merged 44 commits into from
Aug 26, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Aug 18, 2023 •
edited
Loading

Experiment to see if containsLast in HttpFields can be implemented easily for #10326

@gregw gregw requested review from sbordet and lorban August 18, 2023 02:24
@gregw gregw mentioned this pull request Aug 18, 2023
Merged
@gregw
Copy link
Contributor Author

gregw commented Aug 18, 2023

@lorban @sbordet I've added a listIterator(int index) to HttpFields, as this allows for iterating in reverse, which makes finding the last header field much simpler and more efficient. I've updated GzipRequest and containsLast to use this.

But there is something strange going on with HTTP/3 headers. They have a filter mechanism that a) uses streams so is not very efficient; b) is not reflected in the size. I've bypassed the stream for now in the iterators so the size will be correct (probably will break some tests), so this needs careful review.

@gregw
Copy link
Contributor Author

gregw commented Aug 18, 2023

This will be a merge nightmare with #10339 and #10308

@gregw
Only expand request if gzip is last
d9d6dd9
@gregw
extra test
ff73edf 
Signed-off-by: gregw <gregw@webtide.com>
@gregw
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
03459de
@gregw
Reimplemented to use a marked field rather than freeze/thaw
46b11cc
@gregw
Reimplemented to use a marked field rather than freeze/thaw
88cc66c
@gregw
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
9e936ee 
…x/HttpFieldsContainsLast

# Conflicts:
#	jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java
#	jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java
#	jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java
@gregw
Persistent fields are not immutable
64b2722
@gregw
Remove all support in HttpGenerator for Server field
c5cd147
@gregw
Persistent fields are not immutable
e409f44
@gregw
Remove all support in HttpGenerator for Server field
6f6b60b
@gregw
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
999b2b9 
…x/HttpFieldsContainsLast
@gregw
Remove all support in HttpGenerator for Server field
d7e8c7a
@gregw
Fix clear again
7d6d400
@gregw
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
050f351 
…x/HttpFieldsContainsLast

# Conflicts:
#	jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java
@gregw
fixed compile
3057ca5
@gregw
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
7accc62 
…x/HttpFieldsContainsLast
@gregw
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
ddaf2ae
@gregw
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
2a83e03 
…x/HttpFieldsContainsLast
@gregw
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
2b71af4
@gregw
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
42d297f 
…x/HttpFieldsContainsLast
sbordet
sbordet previously approved these changes Aug 24, 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.

Apart a small nit, LGTM.

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ImmutableHttpFields.java
@Override
public HttpField next()
{
if (_index >= _size)
Copy link
Contributor

Choose a reason for hiding this comment

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

There was no change here?

joakime
joakime previously approved these changes 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.

LGTM.

Looks like some of the changes here overlap with those in the Freeze HttpFields PR.
I think we should merge this one first.

@gregw
Copy link
Contributor Author

gregw commented Aug 24, 2023

LGTM.

Looks like some of the changes here overlap with those in the Freeze HttpFields PR. I think we should merge this one first.

This is built on the freeze PR. I thought that would have been first to be approved. Merging this will in effect merge that, but we kind of loose a little bit of history.

@gregw gregw dismissed stale reviews from sbordet and joakime via 7348bf9 August 24, 2023 23:41
@gregw gregw requested review from joakime and sbordet August 24, 2023 23:41
@gregw
Copy link
Contributor Author

gregw commented Aug 25, 2023

This PR should be rebased on top of #10339, shouldn't it? I assumed that and ignored all the changes about PersistentField.

It is. It has been a little hard to keep up to date, as some other reviewed #10339 issues here. I think I have back ported all of those and forward merged now.

sbordet
sbordet approved these changes Aug 25, 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.

A small nit to fix, LGTM.

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MutableHttpFields.java Show resolved Hide resolved
@gregw
Copy link
Contributor Author

gregw commented Aug 25, 2023

Just waiting for a clean build...

lorban
lorban approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

LGTM

@gregw gregw merged commit 8ed56b3 into jetty-12.0.x Aug 26, 2023
2 checks passed
@gregw gregw deleted the experiment/jetty-12.0.x/HttpFieldsContainsLast branch September 13, 2023 22:20
@gregw gregw restored the experiment/jetty-12.0.x/HttpFieldsContainsLast branch September 14, 2023 12:23
@joakime joakime deleted the experiment/jetty-12.0.x/HttpFieldsContainsLast branch September 18, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorban lorban lorban approved these changes

@sbordet sbordet sbordet approved these changes

@lachlan-roberts lachlan-roberts Awaiting requested review from lachlan-roberts

@joakime joakime Awaiting requested review from joakime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
12.0.x
Development

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