Batch Fixes - #2048
Conversation
1) Fix batch dispose issue (error writing to a closed stream) 2) Fix async batch issues (synchronous write to an asynchronous reader error)
| /// <summary> | ||
| /// Release the batch stream and underlying resources | ||
| /// </summary> | ||
| internal void Release() |
There was a problem hiding this comment.
Release() [](start = 22, length = 9)
Detach ? #Resolved
There was a problem hiding this comment.
Well, it's not actually detaching anything; the memory stream is fully contained by the ODataBatchStream; it's just releasing the resources associated with (underlying) stream.
Maybe Free()? Discard? Internal_Dispose?
In reply to: 377796792 [](ancestors = 377796792)
There was a problem hiding this comment.
Went with InternalDispose(), since that's really what's happening.
In reply to: 377919530 [](ancestors = 377919530,377796792)
| HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, requestUri); | ||
| request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json")); | ||
| HttpContent content = new StringContent(@" | ||
| { |
There was a problem hiding this comment.
[](start = 13, length = 104)
a lot of whitespaces are needless. Consider to remove all of them? #Resolved
| new DefaultODataPathHandler(), | ||
| ODataRoutingConventions.CreateDefault(), | ||
| configuration.CreateUnbufferedODataBatchHandler()); | ||
| } |
There was a problem hiding this comment.
revert these changes? #Resolved
|
|
||
| // Act | ||
| HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, requestUri); | ||
| request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json")); |
There was a problem hiding this comment.
request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json")); [](start = 12, length = 86)
this line is mandatory? #Resolved
There was a problem hiding this comment.
In WebAPI, if you don't specify an accept header, the response is multipart/mixed. We should maybe fix this...
In reply to: 377800365 [](ancestors = 377800365)
There was a problem hiding this comment.
Updated WebAPI to set default to JSON if request is JSON and no accept is specified.
In reply to: 377925870 [](ancestors = 377925870,377800365)
| // For responses within a batch, we need to read from the stream after the response is | ||
| // written in order to write it to the batch response stream. So we need to ignore the Close() | ||
| // and provide an alternate method to release the stream after writing its content to the batch response. | ||
| internal class BatchStream : MemoryStream |
There was a problem hiding this comment.
BatchStream [](start = 19, length = 11)
ODataBatchStream? #Resolved
| /// <summary> | ||
| /// Release the batch stream and underlying resources | ||
| /// </summary> | ||
| internal void Release() |
There was a problem hiding this comment.
internal void Release() [](start = 8, length = 23)
shall we add a protect mechanism?
how about to call twice "Release()"?
Maybe test IsClosed() before call base.Close()? #Resolved
f05ffe5 to
a4a7fd4
Compare
-Case-insensitive header comparison -Default to JSON Batch response if request is JSON -Make sure stream is released before reading next request within a batch -Added/fixed tests for standard and .net core
a4a7fd4 to
b217026
Compare
| /// </summary> | ||
| /// <param name="writer">The <see cref="ODataBatchWriter"/>.</param> | ||
| /// <param name="context">The message context.</param> | ||
| public static void WriteMessage(ODataBatchWriter writer, HttpContext context) |
There was a problem hiding this comment.
atic void WriteMessage(ODataBatchWriter writer, HttpContext con [](start = 17, length = 63)
can we call "WriteMessageAsync" in WriteMessage, or Call "WriteMessage()" in "WriteMessageAsync"? #Resolved
There was a problem hiding this comment.
I got rid of WriteMessage, and instead just created a WriteMessageAsync that took a flag to determine whether the writer was async. When we have a major breaking change release of WebAPI OData I want to make all of this internal; there's no need for this functionality to be public, and it's confusing and broken in its current state.
In reply to: 378431693 [](ancestors = 378431693)
…hronous writing behavior to avoid possible (though unlikely) breaking change. Note that any types directly deriving from ODataBatchResponseItem would have to be recompiled due to changes in abstract base class (abstract method now takes a writeAsync parameter). Seems highly unlikely that other custom types are deriving directly from ODataBatchResponseItem. In .NET 5, we should clean up the overloads that write to the stream synchronously, and probably make most of this internal (no reason to make the WriteResponseAsync/WriteMessageAsync on ODataBatchResponseItem public).
Batch Fixes
4 participants
Issues
This pull request fixes issue #2009
Description
In certain batch cases, the stream used to write the response was disposed before all the responses were written, resulting in a closed stream exception.
Also, certain async batch operations called synchronous operations under the covers, resulting in an exception regarding calling synchronous operations after an asynchronous operation.
Checklist (Uncheck if it is not completed)