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 efficiency of mutating of ClientHttpResponse and ServerHttpRequest #24680

Closed
rstoyanchev opened this issue Mar 11, 2020 · 2 comments
Closed

Improve efficiency of mutating of ClientHttpResponse and ServerHttpRequest #24680

rstoyanchev opened this issue Mar 11, 2020 · 2 comments
Assignees
@rstoyanchev
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone
5.3 M1

Comments

@rstoyanchev
Copy link
Contributor

DefaultClientResponse has from(ClientResponse) which copies status, headers, and cookies. It is only half as useful because it ignores the body which is easy to forget or otherwise leads to code like this as a starting point:

ClientResponse.from(response).body(response.bodyToFlux(DataBuffer.class)

We should add ClientResponse.mutate() matching to similar options on the server side. Also both client and server mutate() should be careful not to parse cookies unnecessarily which can lead to surprises, see #24663.

In addition since mutation can be quite common, it should be made as optimal as possible, avoiding object creation in parts not mutated, and also optimizing for repeated use, e.g. response headers).
@rstoyanchev rstoyanchev self-assigned this Mar 11, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Mar 11, 2020
@rstoyanchev rstoyanchev added this to the 5.3 M1 milestone Mar 11, 2020
@rstoyanchev rstoyanchev mentioned this issue Mar 11, 2020
Closed
@SenthilPanneerselvam
Copy link

SenthilPanneerselvam commented Apr 17, 2020 •
edited
Loading

Hi Rossen,
Are you working on this issue? If not I would like to pick this up.
I also have a question related to this, Can you please explain me what does matching to similar options on the server side mean in "We should add ClientResponse.mutate() matching to similar options on the server side."

@rstoyanchev
Copy link
Contributor Author

@SenthilPanneerselvam thanks for the offer to help. I'd afraid the issue is such that it requires a little experimenting but actual changes expected are not much. So by the time I can give you a proper answer I'd probably have the solution more or less ready.

@rstoyanchev rstoyanchev changed the title Improve mutating of DefaultClientResponse and ServerHttpRequest Improve efficiency of mutating of ClientHttpResponse and ServerHttpRequest May 8, 2020
rstoyanchev added a commit that referenced this issue May 11, 2020
@rstoyanchev
Avoid one layer of HttpHeaders wrapping
3276f81 
See gh-24680
rstoyanchev added a commit that referenced this issue May 11, 2020
@rstoyanchev
Avoided repeated creation of ReadOnlyHttpHeaders wrapper
df99889 
See gh-24680
rstoyanchev added a commit that referenced this issue May 11, 2020
@rstoyanchev
Wrappers for native headers on the client side
c908ec1 
Native server request headers have been wrapped rather than copied
since 5.1. This commit applies the same change to the client side
where, to make matters worse, headers were copied repeatedly on every
access, see also previous commit ca897b95.

For Netty and Jetty the wrappers are the same as on the server side
but duplicated in order to remain package private.

See gh-24680
rstoyanchev added a commit that referenced this issue May 11, 2020
@rstoyanchev
Add mutate() to ClientResponse and deprecate from()
67a06f5 
from() has the flaw of ignoring the body and it can't be fixed because
applications are guaranteed to be setting it already and if set twice
the builder drains the first body.

mutate() is a better fit in any case for what needs to be done in a
filter chain. It can be done more efficiently and is consistent with
similar options on the server side.

See gh-24680
rstoyanchev added a commit that referenced this issue May 11, 2020
@rstoyanchev
Shortcut handling of bodyToFlux(DataBuffer.class)
0e9ecb6 
Given that the body is a Flux<DataBuffer> there probably could be a
Flux<DataBuffer> body();

At least bodyToFlux(DataBuffer.class) which is used when mutating and
is a common case should not incur overhead.

See gh-24680
rstoyanchev added a commit that referenced this issue May 11, 2020
@rstoyanchev
Avoid parse cookies when mutating request or response
94824e3 
When mutating a ServerHttpRequest or ClientResponse, the respective
builders no longer access cookies automatically which causes them to
be parsed and does so only if necessary. Likewise re-applying the
read-only HttpHeaders wrapper is avoided.

See gh-24680
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
@rstoyanchev @kenny5he
Avoid one layer of HttpHeaders wrapping
86acc94 
See spring-projectsgh-24680
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
@rstoyanchev @kenny5he
Avoided repeated creation of ReadOnlyHttpHeaders wrapper
1b26b42 
See spring-projectsgh-24680
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
@rstoyanchev @kenny5he
Wrappers for native headers on the client side
5161e92 
Native server request headers have been wrapped rather than copied
since 5.1. This commit applies the same change to the client side
where, to make matters worse, headers were copied repeatedly on every
access, see also previous commit ca897b95.

For Netty and Jetty the wrappers are the same as on the server side
but duplicated in order to remain package private.

See spring-projectsgh-24680
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
@rstoyanchev @kenny5he
Add mutate() to ClientResponse and deprecate from()
39c1f95 
from() has the flaw of ignoring the body and it can't be fixed because
applications are guaranteed to be setting it already and if set twice
the builder drains the first body.

mutate() is a better fit in any case for what needs to be done in a
filter chain. It can be done more efficiently and is consistent with
similar options on the server side.

See spring-projectsgh-24680
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
@rstoyanchev @kenny5he
Shortcut handling of bodyToFlux(DataBuffer.class)
5b7ae66 
Given that the body is a Flux<DataBuffer> there probably could be a
Flux<DataBuffer> body();

At least bodyToFlux(DataBuffer.class) which is used when mutating and
is a common case should not incur overhead.

See spring-projectsgh-24680
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
@rstoyanchev @kenny5he
Avoid parse cookies when mutating request or response
a86a12c 
When mutating a ServerHttpRequest or ClientResponse, the respective
builders no longer access cookies automatically which causes them to
be parsed and does so only if necessary. Likewise re-applying the
read-only HttpHeaders wrapper is avoided.

See spring-projectsgh-24680
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rstoyanchev rstoyanchev

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Milestone
5.3 M1
Development

No branches or pull requests
2 participants
@rstoyanchev @SenthilPanneerselvam