HttpClient sync operations #4776
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
ManickaP
reviewed
Sep 2, 2020
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
carlossanlop
reviewed
Sep 9, 2020
carlossanlop
reviewed
Sep 9, 2020
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
|
Docs Build status updates of commit ced8bab: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
gewarren
requested changes
Sep 21, 2020
pgovind
reviewed
Sep 21, 2020
| <returns>To be added.</returns> | ||
| <remarks>To be added.</remarks> | ||
| <param name="cancellationToken">The cancellation token to cancel the operation.</param> | ||
| <summary>Serializes the HTTP content to a stream using the multipart/* encoding.</summary> |
There was a problem hiding this comment.
the * is subtype from constructor. multipart/* is used across this class doc
There was a problem hiding this comment.
Left some suggestions for you to consider. It looks good, I think we are almost ready to merge.
Also, please make sure to document some 5.0 APIs that were skipped in the files included in this PR:
- System.Net.Http.MultipartContent.HeaderEncodingSelector - Lines 417-419 from file
xml/System.Net.Http/MultipartContent.xml.
And these from xml/System.Net.Http/SocketsHttpHandler.xml:
- System.Net.Http.SocketsHttpHandler.ConnectCallback
- System.Net.Http.SocketsHttpHandler.EnableMultipleHttp2Connections
- System.Net.Http.SocketsHttpHandler.IsSupported
- System.Net.Http.SocketsHttpHandler.KeepAlivePingDelay
- System.Net.Http.SocketsHttpHandler.KeepAlivePingPolicy
- System.Net.Http.SocketsHttpHandler.KeepAlivePingTimeout
- System.Net.Http.SocketsHttpHandler.RequestHeaderEncodingSelector
- System.Net.Http.SocketsHttpHandler.ResponseHeaderEncodingSelector
Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
|
Docs Build status updates of commit 898def7: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
|
Docs Build status updates of commit 71a5890: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
carlossanlop
approved these changes
Sep 22, 2020
ManickaP
reviewed
Sep 23, 2020
There was a problem hiding this comment.
In general main issues:
- Only
HttpClientwithHttpCompletionOption.ResponseContentReadbuffers the response content --> sync method blocks until the whole response is read. In all other cases, even the sync methods return when response headers are read, response content is usually returned as a stream. NotSupportedExceptionis thrown for missing sync implementations everywhere, notNotImplementedException.CreateContentReadStreamoverrides try not to buffer if possible (i.e. return underlying stream directly if possible). Only the base class implementation, the default behavior if not overridden, does the buffering.
| <format type="text/markdown"><![CDATA[ | ||
|
|
||
| ## Remarks | ||
| This operation blocks until the entire response, including content, is read. |
There was a problem hiding this comment.
This is not true for completionOption == ResponseHeadersRead. If it's specified, it blocks only the request sending and response headers reading, not the response content reading.
If the ResponseContentRead is used the remark holds, it blocks till the response content is read.
| <format type="text/markdown"><![CDATA[ | ||
|
|
||
| ## Remarks | ||
| This operation blocks until the entire response, including content, is read or `cancellationToken` cancels the operation. |
There was a problem hiding this comment.
The same as above, depends on the completionOption.
| <param name="stream">The target stream.</param> | ||
| <param name="context">Information about the transport (for example, the channel binding token). This parameter may be <see langword="null" />.</param> | ||
| <param name="cancellationToken">The cancellation token to cancel the operation.</param> | ||
| <summary>When overridden in a derived class, serializes the HTTP content to a stream. Otherwise, throws a <see cref="T:System.NotImplementedException" />.</summary> |
|
|
||
| ]]></format> | ||
| </remarks> | ||
| <exception cref="T:System.NotImplementedException">The method is not overridden in the derived class.</exception> |
| <returns>To be added.</returns> | ||
| <param name="request">The HTTP request message to send.</param> | ||
| <param name="cancellationToken">The cancellation token to cancel operation.</param> | ||
| <summary>When overridden in a derived class, sends an HTTP request with the specified request and cancellation token. Otherwise, throws a <see cref="T:System.NotImplementedException" />.</summary> |
| <format type="text/markdown"><![CDATA[ | ||
|
|
||
| ## Remarks | ||
| This operation blocks until the entire response, including content, is read, or until `cancellationToken` cancels the operation. |
There was a problem hiding this comment.
Handlers do not buffer response content, that's HttpClient functionality.
| <returns>To be added.</returns> | ||
| <remarks>To be added.</remarks> | ||
| <param name="cancellationToken">The cancellation token to cancel the operation.</param> | ||
| <summary>Writes the HTTP stream content to a memory stream.</summary> |
There was a problem hiding this comment.
Not true, also not true for the async version.
| <format type="text/markdown"><![CDATA[ | ||
|
|
||
| ## Remarks | ||
| This operation blocks until all of the content has been written to the memory stream. |
There was a problem hiding this comment.
Not true, all it does is that is just wraps response stream into a readonly one and returns that. It doesn't do any buffering or stream reading.
This looks like the original docs were for the async override were copy-pasted from the base class, because the text applies to the base implementation.
| <format type="text/markdown"><![CDATA[ | ||
|
|
||
| ## Remarks | ||
| This operation blocks until the entire response, including content, is read. |
There was a problem hiding this comment.
Not true, handlers do not buffer response content.
There was a problem hiding this comment.
@carlossanlop some of your team mentioned that we don't explicitly note the method is synchronous. What about remove sentence like "This operation blocks until the entire response, including content, is read." from all remarks?
There was a problem hiding this comment.
As discussed via chat, you can remove the sentence.
| ## Remarks | ||
| This method overrides <xref:System.Net.Http.HttpContent.CreateContentReadStream%2A?displayProperty=nameWithType> to use a custom stream that contains an array, with each HTTP content entity and its boundary encoded and serialized to a <xref:System.IO.MemoryStream> instance. | ||
|
|
||
| This operation blocks until all of the content has been written to the memory stream. |
There was a problem hiding this comment.
Not true if the underlying streams CanSeek (if they can, the above paragraph holds - returning special stream with array of streams). But if the underlying streams cannot seek, the whole process reverts back to the base implementation which buffers all the contents into a MemoryStream.
Merged
gewarren
added a commit
to gewarren/dotnet-api-docs
that referenced
this pull request
Sep 24, 2020
* Draft for @ManickaP * Apply suggestions from code review Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> * Format verification * Format verification * Fix links * Fix links * Fix xml tag * Fix links * Fix links * Fix links * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Fix links * Add CreateContentReadStream * Fix links * Add System.Net.Http.DelegatingHandler.Send * Fix links * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Remove "Synchronous" information from summary * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Fix the exception description Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
carlossanlop
added a commit
that referenced
this pull request
Sep 24, 2020
* Automatic port of System.OperatingSystem docs for RC2 (#4852) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Add Missing System.Diagnostics docs (#4856) * Add Missing System.Diagnostics docs * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Automatically port System.Diagnostics docs for RC2 (#4853) * Automatically port System.Diagnostics docs for RC2 * Update xml/System.Diagnostics/ActivitySamplingResult.xml Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Update xml/System.Diagnostics/Process.xml Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com> * Automatic port of RegexParse* documentation (#4850) * Automatic port of RegexParse* documentation * Update xml/System.Text.RegularExpressions/RegexParseError.xml * Apply suggestions from code review * Update RegexParseException * Update xml/System.Text.RegularExpressions/RegexParseException.xml * Update xml/System.Text.RegularExpressions/RegexParseException.xml Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> * Add ssl API added between preview 7 and 8 (#4860) * Automatic port of Reflection RC2 documentation (#4851) * Revert "Add ssl API added between preview 7 and 8 (#4860)" (#4868) This reverts commit 10d2b88. * Add ssl API added between preview 7 and 8 #2 (#4869) * Add ssl API added between preview 7 and 8 * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * Update xml/System.Net.Security/SslStream.xml * Apply @wfurt comments * Add missing remark headers * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * Documentation for System.Net.Http.SocketsHttpHandler.ConnectCallback (#4861) * Documentation for System.Net.Http.SocketsHttpHandler.ConnectCallback * Apply PR comments * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * Add a remark to AddEvent method (#4871) * Add a remark to AddEvent method * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * Automatic port of CryptoStream docs (#4888) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Automatic port of System.Buffers docs (#4882) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * resolve isprefix conflict * Automatic port of System.Data.Common docs (#4885) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Automatic port of *PipeServerStreamAcl docs (#4880) * Automatic port of *PipeServerStreamAcl docs * Apply suggestions from code review Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> Co-authored-by: David Cantú <dacantu@microsoft.com> * Apply suggestions from code review Co-authored-by: David Cantú <dacantu@microsoft.com> * Apply suggestions from code review * Update xml/System.IO.Pipes/AnonymousPipeServerStreamAcl.xml * Update xml/System.IO.Pipes/AnonymousPipeServerStreamAcl.xml Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> Co-authored-by: David Cantú <dacantu@microsoft.com> * Automatic port of *OSPlatformAttribute docs (#4883) * Automatic port of *OSPlatformAttribute docs * Missing TargetPlatformAttribute constructor parameter description. * Apply suggestions from code review Co-authored-by: Buyaa <buyankhishig.namnan@microsoft.com> * Update xml/System.Runtime.Versioning/SupportedOSPlatformAttribute.xml Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Buyaa <buyankhishig.namnan@microsoft.com> * Document System.Enum (#4894) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Document Half.op_Explicit APIs (#4893) * Document Half.op_Explicit APIs * Equals Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Automatic port of Rune.CompareTo EII (#4891) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Add missing docs for AsnContentException. (#4895) * Add missing documentation for S.S.Cryptography (#4892) * Add missing documentation for S.S.Cryptography * Apply feedback * Automatic port of System.Threading.ExecutionContext.Restore doc (#4887) * Automatic port of System.Threading.ExecutionContext.Restore doc * Update xml/System.Threading/ExecutionContext.xml Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> * Apply suggestions from code review Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> * Automatic port of explicit interface implementation of System.Net.Http.HttpRequestOptions (#4881) * Automatic port of explicit interface implementations for System.Net.Http.HttpRequestOptions * Apply suggestions from code review Co-authored-by: Jan Jahoda <aik.jahoda@post.cz> Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Jan Jahoda <aik.jahoda@post.cz> * Document Type.IsAssignableTo (#4898) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * Document System.PlatformID.Other (#4897) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * HTTP Version Selection (#4870) * Sync docs with source comments * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * Add values Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * HttpClient sync operations (#4776) * Draft for @ManickaP * Apply suggestions from code review Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> * Format verification * Format verification * Fix links * Fix links * Fix xml tag * Fix links * Fix links * Fix links * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Fix links * Add CreateContentReadStream * Fix links * Add System.Net.Http.DelegatingHandler.Send * Fix links * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Remove "Synchronous" information from summary * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Fix the exception description Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> * adding docs for missing 5.0 apis for system.Runtime.CompilerServices.Unsafe (#4902) * adding docs for missing 5.0 apis * Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * resolve conflict * Add Sysytem.Net.* missing API (#4904) * System.Net.Http.SocketsHttpHandler.EnableMultipleHttp2Connections * Add Sysytem.Net.\* missing API * Apply PR comments * Apply suggestions from code review * Apply suggestions from code review * Update xml/System.Net.Http/SocketsHttpHandler.xml Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> * Http sync follow up (#4905) * Add the http sync follow up * Fix closing tag Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com> Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> Co-authored-by: Jan Jahoda <jajahoda@microsoft.com> Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> Co-authored-by: David Cantú <dacantu@microsoft.com> Co-authored-by: Buyaa <buyankhishig.namnan@microsoft.com> Co-authored-by: Jeremy Barton <jbarton@microsoft.com> Co-authored-by: Jan Jahoda <aik.jahoda@post.cz> Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> Co-authored-by: Anirudh Agnihotry <anirudhagnihotry098@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
part of dotnet/runtime#41732