Consume/drain the inbound in case a cancellation is received before subscription #13493
Conversation
kushagraThapar
commented
Jul 24, 2020
•
kushagraThapar
commented
- Consume/drain the inbound in case a cancellation is received before subscription
- Based on the suggestions from reactor-netty owner : SSLException on request cancellation reactor/reactor-netty#1165 (comment)
- Improvements are based on springframework : spring-projects/spring-framework@21d0696
- They further improved it to this : https://github.com/spring-projects/spring-framework/blob/v5.2.8.RELEASE/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpResponse.java
- We also have the same logic in our Rntbd client now
…d before getting subscribed
kushagraThapar
requested review from
David-Noble-at-work,
kirankumarkolli,
mbhaskar and
moderakh
as code owners
ghost
added
the
|
|
||
| private boolean mayHaveBody(HttpMethod method) { | ||
| int code = this.statusCode(); | ||
| return !((code >= 100 && code < 200) || code == 204 || code == 205 || |
There was a problem hiding this comment.
Do 304 classified as non body status code ?
There was a problem hiding this comment.
Yes, 304 will be a non body status code
| * reading was delayed for some reason. | ||
| */ | ||
| private void releaseAfterCancel(HttpMethod method) { | ||
| if (mayHaveBody(method) && this.state.compareAndSet(ReactorNettyResponseState.NOT_SUBSCRIBED, ReactorNettyResponseState.CANCELLED)) { |
There was a problem hiding this comment.
Do we need mayHaveBody check , what happen if we clear on all cancel irrespective of body, we can avoid extra check ?
There was a problem hiding this comment.
Since we are draining content here, we want to make sure we drain it under very specific conditions, specially when the body can be present.
There was a problem hiding this comment.
That is fine ,but my doubt is if some valid response miss mayHaveBody (due to any missed scenario), then we will still face issue , vs draining non body too along with body response (Its a trade off thing )
| private boolean mayHaveBody(HttpMethod method) { | ||
| int code = this.statusCode(); | ||
| return !((code >= 100 && code < 200) || code == 204 || code == 205 || | ||
| method.equals(HttpMethod.HEAD) || headers().getContentLength() == 0); |
There was a problem hiding this comment.
I think some of 4xx error codes from Cosmos (e.g., 400 query plan) also have body.
There was a problem hiding this comment.
Would these be considered as error or success ?
There was a problem hiding this comment.
this condition is already returning true for 4xx, isn't ?
…appened before subscription
|
/check-enforcer override |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@moderakh - I did a quick perf round for Read Latency and Query Clause In Parallel - and the results look similar to master branch. |
|
Ran |
|
|
|
Ran |
|
Ran UniqueIndexTests locally and it passes - fails on the CI - created a github issue - #13532 |
|
Ran |
|
/check-enforcer override |
|
/check-enforcer reset |
|
/check-enforcer override |
