THRIFT-4848: Add ability to set Content-Type,Accept headers in HTTP c… #1801
Conversation
|
I generally welcome that patch, I had in on my todo as well. But I am against the others unrelated stuff. If you think we need that please file another ticket for it and let's discuss these changes separtely. If you could do that, I will commit it (unless I find something else), but in the current form it is a clear -1 from me. |
| { | ||
| if (cancellationToken.IsCancellationRequested) | ||
| if (_isDisposed) |
There was a problem hiding this comment.
Could you comment on that change? Why do we need that and how is it related to setting custom HTTP headers?
| @@ -134,6 +129,11 @@ public override void Close() | |||
|
|
|||
| public override async Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken) | |||
| { | |||
| if (_isDisposed) | |||
There was a problem hiding this comment.
I'd recommend to put that into a method. The CheckDisposed() pattern is already used in some other files.
There was a problem hiding this comment.
Refactoring code is still a good thing. But not in the same PR/ticket :-)
There was a problem hiding this comment.
I'm all for that. I'll look through all the IDisposable classes and and create a ticket to standardize them separately. If there are any other cleanup items that the maintainers want done but don't have time for then feel free to open some thrift tickets and I'll check them out and see how I can help.
|
You're right. Sometimes old habits come back and haunt me. Let me clear out the non-topic specific changes. |
…lient Client: netstd Patch: Kyle Smith
| catch (HttpRequestException wx) | ||
| { | ||
| throw new TTransportException(TTransportException.ExceptionType.Unknown, | ||
| "Couldn't connect to server: " + wx); | ||
| } |
There was a problem hiding this comment.
I have a general problem with nested exception handlers. There are exceptions to the rule (pun intended) but in most cases they are just badly designed and clutter the code towards unmaintainability. In the particular case we have here we have three different tpyes of exceptions which are handled in a strange manner. Is there a chance we can streamline that a bit and consolidate all catches into one level? Or do I overlook sth important?
There was a problem hiding this comment.
Agreed. Looking through the code and I don't see any benefit to the nested try-catch statements. The duplicate catch for HttpRequestException wouldn't ever be hit as far as I could tell. I moved them all into the same level and removed the other nested try-finally since it too could be put in line. Let me know if I went to far.
…lient
Client: netstd
Patch: Kyle Smith
I added the RequestHeaders property that exposes the DefaultRequestHeaders property on the HTTP client instance which allows a developer to set request headers that are not otherwise available using the existing CustomHeaders method. The problem with using CustomHeaders is that it does not allow you to set certain properties (namely Accept). I also added the ContentType property that for specifying the Content-Type request header before sending a request to the server. Our use case is for us to set these headers so that our server can use the correct protocols depending on the clients request.
Other cleanup items include removal of the ConnectTimeout write-only property as the the underlying _connectTimeout field is only ever use on creation, so setting this property was a no-op. I also implemented the OpenAsync and IsOpen fields to work more inline with the other transports.