Support TransferMode.Streamed. #209
Conversation
|
Hi @shmao, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@shmao, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| await xmlWriter.FlushAsync(); | ||
|
|
||
| // The underlying type of xmlWriter, XmlUTF8TextWriter, has not implemented FlushAsync() yet. | ||
| xmlWriter.Flush(); |
There was a problem hiding this comment.
This is really crappy if we suddenly go sync while in a supposedly async path. Can you please open up a bug to track this implementation in the corefx repo?
There was a problem hiding this comment.
Question for @mconnew -- would it be better to await a Task.Run invoking the xmlWriter.Flush()?
There was a problem hiding this comment.
@roncain, I would prefer to keep the call to xmlWriter.FlushAsync and let the xmlWriter decide what to do. If the xmlWriter is doing nothing right now, then that's a bug that needs to be fixed. But the fix could simply be for xmlWriter to either directly call the synchronous Flush method or to call Task.Run(). xmlWriter is the correct place to decide what to do as it is likely easy to check if there's anything to flush and if there isn't, return, otherwise call Task.Run to invoke Flush(). This wouldn't require converting everything to async and would allow a fast path for the no-op situation avoiding starting a new task on another thread just to no-op on that thread and mark the Task complete.
There was a problem hiding this comment.
👍 can we get this calling FlushAsync() instead?
There was a problem hiding this comment.
We can't at this point as the API is not implemented yet.
There was a problem hiding this comment.
The api is not exposed by the XmlWriter contract?
There was a problem hiding this comment.
The API is in the contract, but it’s not implemented yet.
There was a problem hiding this comment.
In that case lets open a bug and give a dummy sync implementation which just returns
Task.FromResult();
There was a problem hiding this comment.
Opened #222 for tracking this.
|
Also, please revise your commit comment prior to merging the PR - omit the "To" |
shmao
changed the title
|
Hi Folks, I've updated the PR. Can you please have a look? Thanks! |
|
Hi @shmao, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| <ProjectReference Include='$(WcfSourceProj)'> | ||
| <Project>{9e50e7bf-cd6e-4269-a6dd-59fd0bd6c0fd}</Project> | ||
| <Name>System.Private.ServiceModel</Name> | ||
| </ProjectReference> |
There was a problem hiding this comment.
Should read:
<!-- Compile tests against the contract, but copy our local-built implementation for testing -->
<ProjectReference Include='$(WcfSourceProj)'>
<Project>{9e50e7bf-cd6e-4269-a6dd-59fd0bd6c0fd}</Project>
<Name>System.Private.ServiceModel</Name>
<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
<OutputItemType>Content</OutputItemType>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Targets>Build;DebugSymbolsProjectOutputGroup</Targets>
</ProjectReference>
unless there's a really good reason for using the P2P references. Using the above ensures that the tests are testing only the surface area that is available in the public contract.
|
|
says: LGTM
|
|
| @@ -80,6 +81,11 @@ internal BodyWriter OnCreateBufferedCopy(int maxBufferSize, XmlDictionaryReaderQ | |||
| } | |||
|
|
|||
| protected abstract void OnWriteBodyContents(XmlDictionaryWriter writer); | |||
| protected virtual Task OnWriteBodyContentsAsync(XmlDictionaryWriter writer) | |||
|
LGTM |
The fix is to enable Streamed transfer mode.
Fixes #7, #37, and #199.