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

Issue #9076 - Cleanups and fixes for multipart in Jetty 12 #9287

Merged
merged 27 commits into from
Feb 9, 2023
Merged

Issue #9076 - Cleanups and fixes for multipart in Jetty 12 #9287

merged 27 commits into from
Feb 9, 2023

Conversation

lachlan-roberts
Copy link
Contributor

Issue #9076

  • Fix parsing bug in multipart (shared backing array for buffers from ServletMultiPartFormData#parse).
  • Add cleanup for temporary parts (save Parts in request attribute for delayed handler and cleanup).
  • Use MAX_FORM_KEYS_KEY and MAX_FORM_CONTENT_SIZE_KEY parameters to configure limits for multipart.
  • Allow all parts to be written to file, multiple writes will instead do a file move, and Part.delete() will delete any file storage for that part.
  • Can use newContentSource() method to get a new Content.Source every time from the part.
  • other fixes and cleanups

@lachlan-roberts
Issue #9066 - parse multipart form data when getParameters is called
da39f93 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
add test to reproduce some issue with multipart
0432817 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
add more tests for multipart
1ff251c 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
fixes to pass new multipart test cases
02f9cc0 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
fix multipart test cases, add more api to the core MultiPart class
3816e4b 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
improvements to multipart
1e20abc 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
improvements to multipart
5bcfeaa 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
539113d 
…9066-MultiPart-getParameters
@lachlan-roberts
Fix testMaxRequest size test in MultiPartServletTest
d4682f4 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
fix some more tests in MultiPartServletTest
2dfe7e6 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
36e7f71 
…9066-MultiPart-getParameters
@lachlan-roberts
changes from review
7387124 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
changes from review
719c60d 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
changes from review
8133dd6 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
changes from review
88c6f20 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
4ffad09 
…9066-MultiPart-getParameters
@lachlan-roberts
fix for retainable merge
02823ec 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
consumeAvailable should use number of reads instead of bytes
d66e36b 
Signed-off-by: Lachlan Roberts <[email protected]>
sbordet
sbordet requested changes Feb 6, 2023
View reviewed changes
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java Outdated Show resolved Hide resolved
...e9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java Outdated Show resolved Hide resolved
jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java
List<Content.Chunk> newChunks = content.stream()
.map(chunk -> Content.Chunk.from(chunk.getByteBuffer().slice(), chunk.isLast()))
.toList();
return new ChunksContentSource(newChunks);
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussing this with @lorban, I think it's best if we store these ChunksContentSource in a List, and we fail them all when this part is closed.

The reason is that in this particular case, if a ChunksContentSource is read after completing the Handler callback, it can read content from buffers that were returned to the pool and may be used for other requests.
By failing them, we guarantee that if they are read after Handler completion, an error is returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if someone calls newContentSource() after the part has been closed, do we need to keep an atomic state and if this has been closed return them a failed instance of Content.Source when they call newContentSource()?

Why can't we just say if this have been closed it is invalid to use it. Just like if you return a RetainableByteBuffer to the pool it is invalid to continue using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #9336

@lachlan-roberts
changes f rom review
d005e97 
Signed-off-by: Lachlan Roberts <[email protected]>
sbordet
sbordet approved these changes Feb 7, 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.

Please fix test failures, and then LGTM.

@lachlan-roberts lachlan-roberts merged commit 3259a55 into jetty-12.0.x Feb 9, 2023
@lachlan-roberts lachlan-roberts deleted the jetty-12.0.x-multipartCleanups branch February 9, 2023 10:25
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Feb 9, 2023
Merge remote-tracking branch 'upstream/jetty-12.0.x' into jetty-12.0.…
2402b6a 
…x-documentation-operations-logging

* upstream/jetty-12.0.x: (35 commits)
  Fixes jetty#9326 - Rename DecryptedEndPoint to SslEndPoint.
  Jetty 10 Upgrade to Hazelcast 5 and totally disable auto join multicast etc.. (fix build on CI) (jetty#9331)
  jetty#9328 - changes from review
  jetty#9287 - catch error in ee9 maxRequestSize MultiPart test
  Jetty 12.0.x 9301 fix ee10 jstl jpms (jetty#9321)
  Issue jetty#9301 Fix dependencies for ee10-glassfish-jstl module (jetty#9303)
  Jetty 12 Hazelcast 5.x and disable auto detection/multicast" (jetty#9332)
  jetty#9287 - fix further test failures
  Fixed imports.
  Issue jetty#7650 - Fix race condition when stopping QueuedThreadPool (jetty#9325)
  jetty#9287 - remove unpaired release of Content.Chunk
  Issue jetty#8991 - rename websocket isDemanding() method to isAutoDemanding()
  Issue jetty#9287 - fix failing tests
  changes f rom review
  add todo to revert to normal pool after fix for jetty#9311
  Issue jetty#9309 - Introducing test for requestlog format with spaces
  use non-pooling RetainableByteBufferPool to work around performance bug
  consumeAvailable should use number of reads instead of bytes
  fix for retainable merge
  changes from review
  ...
lachlan-roberts added a commit that referenced this pull request Feb 13, 2023
@lachlan-roberts
bring back some multipart improvements from #9287
a5344d7 
Signed-off-by: Lachlan Roberts <[email protected]>
This was referenced Feb 13, 2023
Merged
Merged
Closed
Closed
gregw
gregw reviewed Aug 30, 2024
View reviewed changes
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Comment on lines +598 to +599
if (_parser.isTerminated())
throw new RuntimeIOException("Parser is terminated");
Copy link
Contributor

Choose a reason for hiding this comment

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

@lachlan-roberts @sbordet I think this is causing #12212
Can you recall why this was necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gregw this was added in d4682f4, with the commit description "Fix testMaxRequest size test in MultiPartServletTest".

I re-run those tests, and the changes #12213 pass those tests, so I think either was not necessary (maybe just a zealous sanity check), or it is not anymore.

@gregw gregw mentioned this pull request Aug 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw gregw left review comments

@joakime joakime Awaiting requested review from joakime

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
@lachlan-roberts @gregw @sbordet