-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[java] removed usage of FileBackedOutputStream in the client #13308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks @joerg1985!
I think the idea of putting the content to disk is to help session creation request when the user sends a huge Firefox profile. But this is probably fine these days for the reasons you mention in the PR description.
However, I would prefer a review from @shs96c, as he wrote most of this code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #13308 +/- ##
=======================================
Coverage 58.57% 58.57%
=======================================
Files 86 86
Lines 5272 5272
Branches 220 220
=======================================
Hits 3088 3088
Misses 1964 1964
Partials 220 220 ☔ View full report in Codecov by Sentry. |
0d7740b
to
148050a
Compare
148050a
to
a3b8dfd
Compare
CI Failure Feedback(Checks updated until commit c23908e)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where Configuration options
See more information about the |
Description
This will remove the usage of
FileBackedOutputStream
in the client, i don't think it is needed to cache on disk in the client for the following reasons:NewSessionPayload.create(Capabilities caps)
. This might has been different in the days before the W3C protocol was implemented.The server will still use a
FileBackedOutputStream
to keep memory usage low, seeRequestConverter
as this is out of focus from #12737.Please notice:
There is a new interface created in this PR named
Contents.Supplier
. In all areas we can easily ensure the InputStream can be created multiple times and we already know the length of the content. So theContents.memoize
can be dropped which used aFileBackedOutputStream
to ensure the InputStream can be created multiple times.Providing the length does help the HttpClient not to start chunking requests, some drivers do not support this.
This new interface is used by the
HttpMessage
too, the old setter has been deprecated and delegates to the new setter.Motivation and Context
This is part of #12737
Types of changes
Checklist