-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove sync I/O from JsonValueUtils async write methods #2707
Conversation
Debug.Assert(inputString != null, "inputString != null"); | ||
Debug.Assert(buffer != null, "buffer != null"); | ||
|
||
inputString.CopyTo(currentIndex.Value, buffer, 0, substrLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to copyto the buffer with less size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount to copy is determine by the substrLength
. The caller of this method ensures that the substrLength
is not greater than the buffer length: https://github.com/OData/odata.net/pull/2707/files#diff-c86855fdf1be1c1e0a820b8852ccc1fef825001c9ef74274af40778f7adfb96dR452.
Should we add an additional Debug.Assert
in this method to check whether this precondition is violated?
Note that this method is a duplicate of the following existing method: https://github.com/OData/odata.net/pull/2707/files#diff-c86855fdf1be1c1e0a820b8852ccc1fef825001c9ef74274af40778f7adfb96dR780. The only change is that I replaced the ref
parameters with Ref<int>
so that we can use it in async methods.
Co-authored-by: Elizabeth Okerio <elizaokerio@gmail.com>
test/FunctionalTests/Microsoft.OData.Core.Tests/Json/JsonValueUtilsAsyncTests.cs
Outdated
Show resolved
Hide resolved
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
This pull request fixes #2704 .
Description
The following async helper methods from
JsonValueUtils
had cases of synchronous I/O:WriteEscapedJsonStringAsync
WriteEscapedCharArrayAsync
WriteValueAsync(byte[])
Part of the reason was because the methods called other helper methods that performed synchronous
TextWriter.Write
calls. In such cases, I created true async versions of the affected methods.The other reason was that the
NonIndentedTextWriter
did not override the asyncWriteAsync(char[] buffer, int index, int count)
method. And the default implementation ofTextWriter.WriteAsync(char[], int, int)
calls the synchronous version. So I added an async override to the class.Something else what pointing out is that because the internal
StreamWriter
is buffered, it would take a bunch ofWrite()
calls before it actually flushes to the stream. This means that this issue would go undetected in many cases. To test this, I create a long payload to ensure that theStreamWriter
actually flushes its content.Checklist (Uncheck if it is not completed)
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.