ODataMessageWriter can't dispose the stream if there's no write method called #1714
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
called. This is an edge case because customer creates the message writer to write something. However, if there's some control to prevent the message writer writing anything, the outside stream still needs to dispose. This PR is to resolve this edge problem.
xuzhg
requested review from
gathogojr,
KanishManuja-MS,
mikepizzo and
paulodero
xuzhg
added
the
Ready for review
mikepizzo
reviewed
Mar 24, 2020
| @@ -1212,6 +1212,13 @@ private void Dispose(bool disposing) | |||
| { | |||
| this.outputContext.Dispose(); | |||
| } | |||
| else if (!this.writeMethodCalled) | |||
There was a problem hiding this comment.
is the !this.writeMethodCalled check needed? Is there a scenario where outputContext == null and this.writeMethodCalled == true?
| if (this.settings.EnableMessageStreamDisposal) | ||
| { | ||
| this.message.GetStream().Dispose(); | ||
| } |
There was a problem hiding this comment.
Do we need to check for GetStream() returning null?
| @@ -1212,6 +1212,14 @@ private void Dispose(bool disposing) | |||
| { | |||
| this.outputContext.Dispose(); | |||
| } | |||
| else if (this.settings.EnableMessageStreamDisposal) | |||
| { | |||
| Stream stream = this.message.GetStream(); | |||
There was a problem hiding this comment.
Do we need to always call GetStream? Instead, should we keep a reference to it in the ODataMessage and dispose of that if it is not null and call GetStream as part of else.
else if (this.settings.EnableMessageStreamDisposal && this.message.InternalStream != null)
{
this.message.InternalStream.dispose();
}
else if (EnableMessageStreamDisposal)
{
GetStream()
.Dispose();
}
There was a problem hiding this comment.
My concern is the following - I am unsure when and why consumers set this EnableMessageStreamDisposal to false? Perhaps, they do want to use the same stream to write.
Also, I don't think that EnableMessageStreamDisposal is currently being respected for writing, to mitigate the risk, can we make the default value for the writer to be false and have exchange or others opt in but that will make us inconsistent across reader and writer which we cannot afford? Thoughts?
KanishManuja-MS
approved these changes
Mar 24, 2020
KanishManuja-MS
pushed a commit
to KanishManuja-MS/odata.net
that referenced
this pull request
Mar 24, 2020
…d called (OData#1714) * ODataMessageWriter can't dispose the stream if there's no write method called. This is an edge case because customer creates the message writer to write something. However, if there's some control to prevent the message writer writing anything, the outside stream still needs to dispose. This PR is to resolve this edge problem. * Address the comments
KanishManuja-MS
pushed a commit
to KanishManuja-MS/odata.net
that referenced
this pull request
Mar 24, 2020
…d called (OData#1714) * ODataMessageWriter can't dispose the stream if there's no write method called. This is an edge case because customer creates the message writer to write something. However, if there's some control to prevent the message writer writing anything, the outside stream still needs to dispose. This PR is to resolve this edge problem. * Address the comments
KanishManuja-MS
pushed a commit
that referenced
this pull request
Mar 24, 2020
…d called (#1714) * ODataMessageWriter can't dispose the stream if there's no write method called. This is an edge case because customer creates the message writer to write something. However, if there's some control to prevent the message writer writing anything, the outside stream still needs to dispose. This PR is to resolve this edge problem. * Address the comments
KanishManuja-MS
added a commit
to KanishManuja-MS/odata.net
that referenced
this pull request
Mar 25, 2020
…te method called (OData#1714)" This reverts commit a6b0fc3.
KanishManuja-MS
added a commit
to KanishManuja-MS/odata.net
that referenced
this pull request
Apr 20, 2020
…te method called (OData#1714)" This reverts commit 3e02e30.
mikepizzo
added a commit
that referenced
this pull request
May 7, 2020
commit 182964d Author: Sreejith Pazhampilly <[email protected]> Date: Wed May 6 21:39:03 2020 -0700 Fix E2E Tests commit 565ccdc Author: Sreejith Pazhampilly <[email protected]> Date: Thu Apr 30 11:03:23 2020 -0700 Update to net core 2.1 and 3.1 for UT commit afeb7d8 Author: Sreejith Pazhampilly <[email protected]> Date: Tue Apr 28 16:33:36 2020 -0700 Make the CI working commit d844d4a Author: Sam Xu <[email protected]> Date: Thu Apr 23 22:09:17 2020 -0700 Update the test case projects commit 26709dd Author: Sreejith Pazhampilly <[email protected]> Date: Thu Apr 23 12:09:39 2020 -0700 updates , pipeline changes commit 21a1d3d Author: Sam Xu <[email protected]> Date: Wed Apr 22 18:27:12 2020 -0700 Modify the unit test case, fix the waring, fix the version, etc commit 82f949d Author: Sam Xu <[email protected]> Date: Wed Apr 22 15:01:40 2020 -0700 update the build CI and project commit c9fae43 Author: Sam Xu <[email protected]> Date: Tue Apr 21 15:21:30 2020 -0700 Clean up the codes commit 6c50be0 Author: Sam Xu <[email protected]> Date: Tue Apr 21 14:56:13 2020 -0700 With whole project changes commit 77d9f88 Author: Paul Odero <[email protected]> Date: Thu May 7 10:52:23 2020 +0300 Fix #1756 (#1764)- Reading OData Error Response in OData Client commit 5a20f51 Author: Sam Xu <[email protected]> Date: Mon May 4 16:10:36 2020 -0700 Resolve the product code build warnings commit 26739c1 Author: Sam Xu <[email protected]> Date: Mon May 4 13:48:50 2020 -0700 Fix WebApi issue OData/WebApi#2136: IN operator with double quote fails commit 44ed8db Author: Paul Odero <[email protected]> Date: Mon Apr 27 11:52:23 2020 +0300 Fixing issue #794 (#1656) commit e0e628a Author: Paul Odero <[email protected]> Date: Mon Apr 27 09:36:08 2020 +0300 Enable OData client to send IEEE754Compatible parameter in the reques… (#1659) * Fixes #725 and also fixes #522 commit a398670 Author: Sam Xu <[email protected]> Date: Fri Apr 17 11:48:34 2020 -0700 Fix some build warning in Edm lib commit 82ed887 Author: Clément Habinshuti <[email protected]> Date: Wed Apr 15 13:39:56 2020 +0300 Add support for relative uris and absolute uris with host header in json batch requests (#1740) commit b17d455 Author: Clément Habinshuti <[email protected]> Date: Wed Apr 15 11:40:01 2020 +0300 Update error message when adding unsupported query option (#1729) * Update error message when adding unsupported query option * Update string resources in portablelib * Fix resource string errors in portable lib * Update error message * Remove references to unused error message * Fix failing tests * Fix typo commit d3769fd Author: John Gathogo <[email protected]> Date: Thu Apr 9 10:55:38 2020 +0300 Fix bug where open types are not identified as such during serialization (#1727) * Fix bug where open types are not identified as such during serialization Remove unnecessary assert * serviceModel may be initialized from 2 locations. Ensure both trigger population of edm structured elements Co-authored-by: John Gathogo <[email protected]> commit 80e73d0 Author: KanishManuja-MS <[email protected]> Date: Fri Apr 3 10:10:43 2020 -0700 Revert "ODataMessageWriter can't dispose the stream if there's no write method called (#1714)" This reverts commit 3e02e30. # Conflicts: # test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/Writer/JsonLight/ODataJsonLightInheritComplexCollectionWriterTests.cs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues
This pull request fixes issue #xxx.
Description
Exchange reports an issue that:
Looks like there are certain conditions under which ODataMessageWriter is not disposing the stream returned by IODataResponseMessage.GetStream. Are there any known bugs in this area?
After investigation, we found this edge case. This is an edge case only happens when customer creates the message writer without writing anything. However, if there's some control to prevent the
message writer from writing anything, the outside stream still needs to
dispose. This PR is to resolve this edge problem.
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.