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
Implement asynchronous support in ODataNotificationReader #2324
Implement asynchronous support in ODataNotificationReader #2324
Conversation
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.
I have some concerns regarding the ownership of the wrapped TextReader
. You mention in the comments that it should not be disposed by the ODataNotificationReader
since it's not the owner. However, in the original version, the ODataNotificationReader
did dispose of the TextReader
. Can you confirm that your changes are the expected behaviour and the previous implementation was a bug?
I have found the following scenario where an ODataNotificationReader
is created:
public override sealed TextReader CreateTextReader()
{
if (this.State == ODataReaderState.Stream)
{
/* ... omitted for brevity */
return new ODataNotificationReader(this.InterceptException(this.CreateTextReaderImplementation), this);
}
else
{
throw new ODataException(Strings.ODataReaderCore_CreateTextReaderCalledInInvalidState);
}
}
Here, a TextReader
is created inline and passed directly to the ODataNotificationReader
constructor. There's no other reference left for the TextReader
. So if the ODataNotificationReader
does not dispose it, then TextReader
will not be disposed as there's no other object which has a reference to it. And the calling method will only get back a reference of the ODataNotificationReader
since that's the return value, and will have no idea that it's wrapping another TextReader
given that the return type is simply TextReader
.
If ODataNotificationReader
is not the owner, then that logic should be refactored such to ensure TextReader
gets properly disposed. If ODataNotificationReader
is the owner (and the above snippet suggests that it is), then it should dispose of TextReader
. In either case, I think the doc comments of ODataNotificationReader
should be updated to mention whether or not it assumes the ownership responsibility of disposing TextReader
.
test/FunctionalTests/Microsoft.OData.Core.Tests/ODataNotificationReaderTests.cs
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/ODataNotificationReaderTests.cs
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/ODataNotificationReaderTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/ODataNotificationReaderTests.cs
Outdated
Show resolved
Hide resolved
e4aa83a
to
89e3524
Compare
59c914e
to
2870352
Compare
2870352
to
a8ebf56
Compare
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 partially fulfills #2019.
Description
Implement asynchronous support in
ODataNotificationReader
.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.