Implement asynchronous support in JsonWriter by gathogojr · Pull Request #2014 · OData/odata.net

gathogojr · 2021-03-02T09:24:54Z

Issues

This pull request is a partial fulfilment of issue #2019.

Description

Implement asynchronous support in JsonWriter

Added IJsonWriterAsync interface.
Added IJsonStreamWriterAsync interface.
Added IJsonWriterFactoryAsync interface.
Implemented IJsonStreamWriterAsync asynchronous methods in JsonWriter class.
Implemented IJsonWriterFactoryAsync methods in DefaultJsonWriterFactory class.
Implemented asynchronous methods in ODataJsonTextWriter class.
Implemented asynchronous methods in ODataBinaryStreamWriter class.
Added test for JsonWriter, ODataJsonTextWriter and ODataBinaryStreamWriter classes asynchronous methods.
Passing around a char array reference in the synchronous workflow to support use of char array pool and a char array wrapped in an object in the asynchronous workflow started degenerating into a code smell so I opted for one that works across both workflows.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

habbes · 2021-03-10T14:09:08Z

        /// Char buffer to use for streaming data.
        /// Array pool for renting a buffer.
-        internal static void WriteValue(TextWriter writer, string value, ODataStringEscapeOption stringEscapeOption, ref char[] buffer, ICharArrayPool arrayPool = null)
+        internal static void WriteValue(TextWriter writer, string value, ODataStringEscapeOption stringEscapeOption, Ref<char[]> buffer, ICharArrayPool arrayPool = null)


Does the use Ref affect memory allocations or cleanup when using the sync writer?

@habbes Passing around a char array reference in the synchronous workflow to support use of char array pool and a char array wrapped in an object in the asynchronous workflow started degenerating into a code smell so I opted for one that works across both workflows.
The previous approach required me to maintain both a char[] buffer and a Ref buffer as member variables of the JsonWriter class, both renting char arrays from the same pool, and returning to the same pool. The code felt brittle and likely to hide subtle bugs.
So the answer is yes, with this change both use the wrapped buffer.

habbes · 2021-03-10T14:52:22Z

+            Debug.Assert(this.scopes.Count > 0, "No scope to end.");
+
+            await this.writer.WriteLineAsync().ConfigureAwait(false);
+            await this.writer.DecreaseIndentationAsync().ConfigureAwait(false);


Does the DecreaseIndentationAsync() actually write anything? Should it be async?

@habbes Depending on the writer implementation, it might or might not write anything. For this reason, it's right to await. It just happens that for NonIndentedTextWriter it doesn't write anything.

I understand that argument for WriteLineAsync, but I guess I just couldn't imagine an implementation of increasing/decreasing indentation that would need to write to the stream. I assume it would update the indentation level, which would probably be stored in an int, then other Writexxx methods would read the indentation level to decide how many spaces to add after a new line. Maybe I'm making too much assumptions here. But I wonder what the difference in overhead and resource cost there is compared to if it were not async.

@habbes On the contrary, since WriteAsync/WriteValueAsync methods do not concern themselves with whitespace characters, the typical implementation of DecreaseIndentationAsync would be responsible for working out the applicable indentation level while also writing the whitespace spaces required to apply the indentation.
For example, in EndObjectScopeAsync method, we do:

WriteLineAsync() - Apply a new line

DecreaseIndentationAsync() - Decrease indentation level and apply indentation

WriteAsync(scope.EndString) - Write the } character to signal end of the object
From the above you can see that DecreaseIndentationAsync would have to be responsible for both decreasing indentation level and applying the indentation itself.

Hmm I see what you mean. But here is where my confusion comes from.
IncreaseIndentationAsync() and DecreaseIndentationAsync() seem to only be called once per "block" (where a block is a series of "statements" that should appear at the same indentation level).

If the indentation method was responsible for writing the spaces, then it would be called every time before writing a property for example, or every time before writing an array element. Is that currently the case? Looking at the usage, it seems that the indentation methods are only called at the beginning and end of an object, and not at the properties in between. I could be wrong. And this could be irrelevant at the end of the day. But could be worth finding out.

Maybe it could make sense to measure whether making IncreaseIndentation async vs sync makes a difference. If it doesn't make a difference then I think you could ignore my concern. If it does make a difference, it could be worth looking into because once this method is merged in the public API, it might be hard to change later on if we find that we need to, since the signature would have to change.

WebAPI JSON responses are indented, maybe we could see what implementation it uses, that could provide insights on how the Increase/DecreaseIndentation methods are meant to achieve. But if in doubt, I think making them async is the safer thing to do.

Actually, just remembered that they are not indented, it's just that the postman client and browser extensions auto-format the response. My bad.

@habbes

indentation methods are only called at the beginning and end of an object, and not at the properties in between

This is indeed the case. The indentation methods are called at the beginning of an array, object and padding function. The JsonWriter in ODL does not apply a newline or indentation before a property is written

habbes · 2021-03-10T14:59:04Z

+            await this.writer.WriteLineAsync().ConfigureAwait(false);
+            await this.writer.DecreaseIndentationAsync().ConfigureAwait(false);
+            Scope scope = this.scopes.Pop();
+
+            Debug.Assert(scope.Type == ScopeType.Object, "Ending scope does not match.");
+
+            await this.writer.WriteAsync(scope.EndString).ConfigureAwait(false);


I assume DecreaseIndentationAsync doesn't actually write anything. In which case, isn't it a waste of resources to make it async?
And if it doesn't write anything, can the WriteLine and Write of the scope.EndString be combined in one async call?

See my comment here

habbes · 2021-03-10T15:06:57Z

+            Debug.Assert(this.scopes.Peek().Type == ScopeType.Object, "The active scope must be an object scope for name to be written.");
+
+            Scope currentScope = this.scopes.Peek();
+            if (currentScope.ObjectCount != 0)


Does ObjectCount refer to the number of properties in the object?

@habbes Yes. In this case for example, we're verifying that we're within an object scope when writing a property name.

habbes · 2021-03-10T17:32:25Z

+            // IMPORTANT: The Dispose method of the returned stream does the following:
+            // - Writes trailing bytes to the writer synchronously
+            // - Flushes the buffer of the writer synchronously
+            // ODL supports net45 and netstandard1.1 (in addition to .netstandard2.0)
+            // This makes it complicated to implement IAsyncDisposable in ODataBinaryStreamWriter
+            // TODO: Can the returned stream be safely used asynchronously?
+            this.binaryValueStream = new ODataBinaryStreamWriter(writer, this.wrappedBuffer, ArrayPool);
+            return this.binaryValueStream;


If writing and flushing synchronously when disposing is a great concern, maybe you could extend the ODataBinaryStreamWriter with a version where Dispose() simply calls the base.Dispose() then you add a method like WriteTrailingBytesAndFlushAsync(). Then you call:

await (this.binaryValueStream as ODataBinaryStreamWriterAsync).WriteTrailingBytesAndFlushAsync(); this.binaryValueStream.Dispose(); this.binaryValueStream = null;

But this looks messy and maybe not worth it if it will be replaced when dropping net45.

But since the binaryValueStream property is private implementation detail, maybe there's no harm in changing its type to some custom AsyncDisposableStream that extends Stream and adds a DisposeAsync method. Then make ODataBinaryStreamWriterAsync extend that type. Then when net45 is dropped, you could extend a regular Stream.

Or what about an extension method to the Stream class wrapped instead #if NET45?

habbes · 2021-03-10T22:03:06Z

        {
-            return TaskUtils.GetTaskForSynchronousOperation(() =>
-                this.Write(value));
+            await this.WriteEscapedCharValueAsync(value).ConfigureAwait(false);


Is it necessary to async/await this given that the method being called already returns an async task? Or it doesn't make a difference? I assume we incur more overhead in this case compared to just returning the task? I could be wrong.

According to the discussion here: https://stackoverflow.com/questions/38017016/async-task-then-await-task-vs-task-then-return-task the differences lie in exception handling and the state machine generated for async/await by the compiler. The discussions also suggest that there are optimizations to prevent unnecessary awaiting. But according to the linked sharplab code sample it appears that the state machine instance is generated regardless. I think this could lead to unnecessary allocations.

My thoughts are that we don't need async here.
But am not sure if there is extra overhead by having async

@habbes @KenitoInc Eliding async and await can lead to unexpected pitfalls/behaviour. Reference to this article plus Stephen Toub's article on the cost of async and await, the state machine for async methods will capture exceptions from your code and place them on the returned task. Without the async keyword, the exception is raised directly rather than going on the task:

public async Task<string> GetWithKeywordsAsync() { string url = /* Something that can throw an exception */; return await DownloadStringAsync(url); } public Task<string> GetElidingKeywordsAsync() { string url = /* Something that can throw an exception */; return DownloadStringAsync(url); }

These methods work exactly the same as long as the calling method does something like this:

var result = await GetWithKeywordsAsync(); var result = await GetElidingKeywordsAsync();

However, if the method call is separated from the await, then the semantics are different:

var task = GetWithKeywordsAsync(); var result = await task; // Exception thrown here var task = GetElidingKeywordsAsync(); // Exception thrown here var result = await task;

Eliding the keywords in this case causes different (and unexpected) exception behavior.

Honestly I'd rather focus on micro optimizations separately after a analyzing the costs carefully plus first determining whether the compiler does those kind of optimizations by itself.
Plus I'd hope to receive views on the same from other reviewers before embarking on those kind of manual interventions.

But in his particular case we are not doing anything that could throw an exception before the await. We're simply fowarding the arguments as they are to an async call, with no intermerdiary checks or preprocessing calls. Also, since this that's called by another async method with will be called using async/await, any exception that it would throw would be caught by the async method that calls it, so it wouldn't end being unhandled. Aside from the optimization concern, I was just wondering why it was necessary to have the await there in the first place.

But if you feel like it's risky to remove it, I have not objections to keeping it there.

habbes · 2021-03-10T22:12:38Z

+            // if we have less than 3 bytes, store the bytes and continue
+            if (count + trailingBytes.Length < MinBytesPerWriteEvent)
+            {
+                this.trailingBytes = this.trailingBytes.Concat(bytes.Skip(offset).Take(count)).ToArray();


Maybe this is not the scope of this PR, but just curious whether reallocating the trailingBytes all the time doesn't put pressure on the GC? Can a single array with an index to keep track of where to add new bytes be used? Or an array pool?

And the impact of the LINQ calls?

@habbes I stumbled upon this method and my thoughts were the same as yours. I did a refactor to make some of the logic shareable between Write and WriteAsync method.
What I believe is needed is a rewrite taking into consideration the suggestions you have provided. I even left a comment to that effect in the PrepareByteArray method where I moved the shareable code.
I'll have to make a judgement whether to create a separate issue for that or make that change in the PR. That said, I don't think this method is in a hot path based on the typical scenarios that this class is applied.

KenitoInc

A few comments

KenitoInc · 2021-03-12T09:11:26Z

+    /// 

            
+    /// Interface for writing JSON including streaming binary values.
+    /// 
+    [CLSCompliant(false)]


Why do we have this attribute?

@KenitoInc I adopted this from the existing equivalent class IJsonStreamWriter. The attribute indicates whether a program element is compliant with the Common Language Specification (CLS).

KenitoInc · 2021-03-12T13:10:30Z

        {
-            return TaskUtils.GetTaskForSynchronousOperation(() =>
-                this.Write(value));
+            await this.WriteEscapedCharValueAsync(value).ConfigureAwait(false);


My thoughts are that we don't need async here.
But am not sure if there is extra overhead by having async

xuzhg · 2021-03-17T17:29:10Z

+        /// Writer to which text needs to be written.
+        /// True if it is IEEE754Compatible.
+        /// The JSON writer created.
+        IJsonWriterAsync CreateAsyncJsonWriter(TextWriter textWriter, bool isIeee754Compatible);


CreateAsyncJsonWriter [](start = 25, length = 21)

What's the reason not to name it as "CreateJsonWriterAsync"?

@xuzhg I considered that and at one point I had even named the method that way. However, methods suffixed with Async usually suggests that a return type is Task or Task while this method just returns a JSON writer that supports writing asynchronously. I looked through the repo and came across methods like CreateAsynchronousReader and thought I could adopt the similar naming here.

Maybe renaming to CreateAsynchronousJsonWriter could avoid the confusion and bring more consistency with CreateAsynchronousReader.

Or maybe it would be too long?

@habbes Done

xuzhg

KenitoInc

LGTM

gathogojr force-pushed the feature/jsonwriter-async-api branch 3 times, most recently from 4d4f81e to 3be89b2 Compare March 5, 2021 08:50

gathogojr force-pushed the feature/jsonwriter-async-api branch from cd8526b to 467782c Compare March 10, 2021 09:45

gathogojr requested review from ElizabethOkerio, KenitoInc, habbes, marabooy, mikepizzo, odero and xuzhg March 10, 2021 09:47

gathogojr force-pushed the feature/jsonwriter-async-api branch from cb26215 to d05f478 Compare March 10, 2021 13:16

habbes reviewed Mar 10, 2021

View reviewed changes

Comment thread src/Microsoft.OData.Core/Json/JsonWriter.cs Outdated

habbes reviewed Mar 10, 2021

View reviewed changes

KenitoInc reviewed Mar 12, 2021

View reviewed changes

gathogojr requested review from KenitoInc and habbes March 15, 2021 07:31

gathogojr force-pushed the feature/jsonwriter-async-api branch from 5dcc714 to 55dca12 Compare March 16, 2021 08:06

marabooy reviewed Mar 17, 2021

View reviewed changes

Comment thread src/Microsoft.OData.Core/Json/JsonWriterAsyncExtensions.cs Outdated

Implement JsonWriter asynchronous workflow

80f7ead

gathogojr force-pushed the feature/jsonwriter-async-api branch from 55dca12 to 80f7ead Compare March 17, 2021 13:23

xuzhg reviewed Mar 17, 2021

View reviewed changes

xuzhg approved these changes Mar 17, 2021

View reviewed changes

KenitoInc approved these changes Mar 19, 2021

View reviewed changes

John Gathogo added 3 commits March 19, 2021 21:41

Fix naming to avoid ambiguity

6eab65f

Minor formatting changes

46fcb74

Make suggested naming change

b5f1605

marabooy reviewed Mar 23, 2021

View reviewed changes

Comment thread src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs Outdated

marabooy reviewed Mar 23, 2021

View reviewed changes

Comment thread src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs Outdated

marabooy approved these changes Mar 23, 2021

View reviewed changes

Wrap parameters for cases where lines are too long

cf7d02e

gathogojr merged commit 25ea344 into OData:master Mar 23, 2021

gathogojr deleted the feature/jsonwriter-async-api branch March 23, 2021 08:26

Conversation

gathogojr commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Uh oh!

habbes Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

habbes Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

habbes Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

habbes Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KenitoInc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr commented Mar 2, 2021 •

edited

Loading

habbes Mar 10, 2021 •

edited

Loading

gathogojr Mar 10, 2021 •

edited

Loading

habbes Mar 10, 2021 •

edited

Loading

habbes Mar 22, 2021 •

edited

Loading

gathogojr Mar 23, 2021 •

edited

Loading

habbes Mar 10, 2021 •

edited

Loading