KeepAliveEnabled property implementation for HttpTransportBindingElement #2370
Conversation
|
👍 LGTM |
|
@BogdanovKirill Thanks for your interest in contributing to the project. As noted in our contributing doc, WCF repo follows the .NET Core Contribution guidelines. For API additions, we would recommend not to submit PRs until the APs have been approved via the API Review Process. Can you please open an issue and follow the said process? |
|
Your implementation looks correct. Would you be able to add a test? You can add a variation to this source file. Add a new method to this interface and this class. In the implementation, get the request headers using the same mechanism as used here and check the Connection header is present with the value "close". |
| property = obj as HttpRequestMessageProperty; | ||
| WebHeaderCollection collection = property.Headers; | ||
| string connectionValue = collection.Get(Enum.GetName(typeof(HttpRequestHeader), HttpRequestHeader.Connection)); | ||
| return connectionValue.Equals("Close", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Would nameof(HttpRequestHeader.Connection) work here?
| WebHeaderCollection collection = property.Headers; | ||
| string connectionValue = collection.Get(Enum.GetName(typeof(HttpRequestHeader), HttpRequestHeader.Connection)); | ||
| return connectionValue.Equals("Close", StringComparison.OrdinalIgnoreCase); | ||
| } |
There was a problem hiding this comment.
Doesn't this return true if the Connection header has the value Close meaning KeepAlive is disabled? Also this method doesn't echo anything back so having Echo in the name doesn't make sense. Can you either rename to IsKeepAliveEnabled and flip the logic or rename to IsKeepAliveDisabled. You are also returning true if there is a missing message property. Instead throw an exception as in this case we can't tell. This will be converted to a FaultException on the client so will signal something has gone wrong with the test scenario.
|
Thank you very much for help. Need some time, I will fix my mistakes |
|
Could you check new iteration please ? |
This property could be used to disable for each HTTP query keep-alive header.
|
Sorry, Christmas got away from me. The code looks good, I'm going to manually verify this locally rebased against master and if everything looks good, then I'll merge it. |
|
Fixes #2389 |
This property could be used to disable for each HTTP query keep-alive header.