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.

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

clear http response inputstream and errorstream to reuse connection. #863

Merged
merged 2 commits into from
Dec 16, 2017

Conversation

yllions
Copy link

@yllions yllions commented Dec 4, 2017

I missed some cases in the previous pull request. It is when server status code is more than 400, you will get an excepion if you try to open input stream. I have fixed it in this pull request.

  1. you should test scenarios when server response code is more than 400. In my test, this patch can dramatically decrease connections.
  2. In normal cases, you will get null if you try to get error inputstream and it is ok.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 8d31e51 on xiaoerlyl:keepalive into ** on ctripcorp:master**.

@codecov-io
Copy link

codecov-io commented Dec 4, 2017

Codecov Report

Merging #863 into master will increase coverage by 0.03%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #863      +/-   ##
============================================
+ Coverage      46.9%   46.93%   +0.03%     
  Complexity     1544     1544              
============================================
  Files           352      352              
  Lines          9703     9715      +12     
  Branches        961      962       +1     
============================================
+ Hits           4551     4560       +9     
- Misses         4811     4812       +1     
- Partials        341      343       +2
Impacted Files Coverage Δ Complexity Δ
...com/ctrip/framework/apollo/util/http/HttpUtil.java 85.71% <84.61%> (-0.78%) 7 <0> (+1)
...ervice/service/ReleaseMessageServiceWithCache.java 84.52% <0%> (-1.2%) 23% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0bbf0d...b2f24cb. Read the comment docs.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Thanks for your detailed explanation and fix!

In Apollo cases, the normal response code will only be 200 and 304, so is it appropriate to open isr when status code is 200 and 304, and open esr otherwise?

The code may look a little bit cleaner in that way. But I have not tested yet.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 50.468% when pulling b2f24cb on xiaoerlyl:keepalive into d0bbf0d on ctripcorp:master.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Thanks, I just tested the scenarios you mentioned, and it does work!

@nobodyiam nobodyiam merged commit f9cb281 into apolloconfig:master Dec 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants