-
Notifications
You must be signed in to change notification settings - Fork 128
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
Curl keep alive #5930
base: main
Are you sure you want to change the base?
Curl keep alive #5930
Conversation
Does this fix address issue 5877 |
Co-authored-by: Rick Winter <rick.winter@microsoft.com>
I have a few comments, but my biggest concern is that we don't have any tests which validate this code against an actual server which returns keep-alive responses, thus there may be assumptions being made in this implementation which are not supported in actual practice. Do we know if storage or other servers we use returns keep-alive headers which we could use to help test? |
since i agree with this statement, i don't think we should merge this change, even if on surface it might look ok, i'd rather not merge it since we cannot get real life validation of the approach. as such i'll leave it there if someone else wants to take a stab. Also since my vacation is coming up i don't want to merge something of this impact and then vanish for a few weeks. |
return response; | ||
} | ||
|
||
// Check if the server supports keep alive and if the headers are consistent between the request and | ||
// the response. If they are not consistent, the keep alive header in the request is removed.and the |
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.
// the response. If they are not consistent, the keep alive header in the request is removed.and the | |
// the response. If they are not consistent, the keep alive header in the request is removed and the |
// just in case the server sends the headers in a different case, the case sensitivity of the | ||
// map is guaranteed for keys not values. So we need to ensure we compare the values in a case | ||
// insensitive way. |
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.
Nit,
// just in case the server sends the headers in a different case, the case sensitivity of the | |
// map is guaranteed for keys not values. So we need to ensure we compare the values in a case | |
// insensitive way. | |
// Case sensitivity only applies to the `Key` in the map. Thus. compare `Value` in a case insensitive manor. |
{ | ||
// Can't re-used a shut down connection | ||
// Can't re-used a shut down connection or an expired connection |
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.
// Can't re-used a shut down connection or an expired connection | |
// Can't re-use a shutdown connection or an expired connection |
/azp run cpp - core - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
API change check APIView has identified API level changes in this PR and created following API reviews. |
closes #5877
Pull Request Checklist
Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:
See the detailed list in the contributing guide.