Skip to content
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.

Sign up for GitHub

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

ODataNotificationWriter and ODataNotificationStream wrapper classes should not dispose the passed TextWriter and Stream respectively #2110

Merged
merged 1 commit into from
Jun 18, 2021
Merged

ODataNotificationWriter and ODataNotificationStream wrapper classes should not dispose the passed TextWriter and Stream respectively #2110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Microsoft.OData.Core/ODataNotificationStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,7 @@ protected override void Dispose(bool disposing)
}

this.listener = null;

this.stream?.Dispose();
// NOTE: Do not dispose the stream since this instance does not own it.
this.stream = null;
}

Expand All @@ -235,9 +234,9 @@ public async ValueTask DisposeAsync()
{
await this.listener.StreamDisposedAsync()
.ConfigureAwait(false);
this.stream?.Dispose();

this.listener = null;
// NOTE: Do not dispose the stream since this instance does not own it.
this.stream = null;
}

Expand Down
5 changes: 2 additions & 3 deletions src/Microsoft.OData.Core/ODataNotificationWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,7 @@ protected override void Dispose(bool disposing)
}

this.listener = null;

this.textWriter?.Dispose();
// NOTE: Do not dispose the text writer since this instance does not own it.
this.textWriter = null;
}

Expand All @@ -356,9 +355,9 @@ public async ValueTask DisposeAsync()
{
await this.listener.StreamDisposedAsync()
.ConfigureAwait(false);
this.textWriter?.Dispose();

this.listener = null;
// NOTE: Do not dispose the text writer since this instance does not own it.
this.textWriter = null;
}

Expand Down
10 changes: 5 additions & 5 deletions test/FunctionalTests/Microsoft.OData.Core.Tests/ODataNotificationStreamTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public void NotificationStreamDisposeShouldInvokeStreamDisposed(bool synchronous
// We care about the notification stream being disposed
// We don't care about the stream passed to the notification stream
using (var notificationStream = new ODataNotificationStream(
new MemoryStream(),
this.stream,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change necessary? Is this.stream used in the test? Do you make a check to verify that it was not disposed?

Copy link
Contributor Author

@gathogojr gathogojr Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please observe that I'm verifying that StreamDisposed or StreamDiposedAsync are invoked by reading the content of the stream they wrote to when they were invoked. In the previous case, StreamDisposed/StreamDiposedAsync were writing to a stream entirely different to the one passed to ODataNotificationStream constructor. Since ODataNotifictionStream is not disposing the stream after the change, I was able to make StreamDisposed/StreamDiposedAsync write to the same stream passed to the constructor with the assurance that it'll still be available for the the helper ReadStreamContents/ReadStreamContentsAsync methods to read from. That is why I noted in the PR description that the change I made to the tests is the confirmation needed to verify that the stream is not disposed

this.streamListener,
synchronous))
{
Expand All @@ -49,7 +49,7 @@ public void NotificationStreamDisposeShouldInvokeStreamDisposed(bool synchronous
public void NotificationStreamDisposeShouldBeIdempotent(bool synchronous, string expected)
{
var notificationStream = new ODataNotificationStream(
new MemoryStream(),
this.stream,
this.streamListener,
synchronous);

Expand All @@ -69,7 +69,7 @@ public void NotificationStreamDisposeShouldBeIdempotent(bool synchronous, string
public async Task NotificationStreamDisposeShouldInvokeStreamDisposedAsync()
{
await using (var notificationStream = new ODataNotificationStream(
new MemoryStream(),
this.stream,
this.streamListener)) // `synchronous` argument becomes irrelevant
{
}
Expand All @@ -83,7 +83,7 @@ public async Task NotificationStreamDisposeShouldInvokeStreamDisposedAsync()
public async Task NotificationStreamDisposeAsyncShouldBeIdempotent()
{
var notificationStream = new ODataNotificationStream(
new MemoryStream(),
this.stream,
this.streamListener);

// 1st call to DisposeAsync
Expand All @@ -102,7 +102,7 @@ public async Task NotificationStreamDisposeAsyncShouldBeIdempotent()
public async Task NotificationStreamDisposeShouldInvokeStreamDisposedAsync()
{
using (var notificationStream = new ODataNotificationStream(
new MemoryStream(),
this.stream,
this.streamListener,
/*synchronous*/ false))
{
Expand Down
10 changes: 5 additions & 5 deletions test/FunctionalTests/Microsoft.OData.Core.Tests/ODataNotificationWriterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public void NotificationWriterDisposeShouldInvokeStreamDisposed(bool synchronous
// We care about the notification writer being disposed
// We don't care about the writer passed to the notification writer
using (var notificationWriter = new ODataNotificationWriter(
new StreamWriter(new MemoryStream()),
this.writer,
this.streamListener,
synchronous))
{
Expand All @@ -49,7 +49,7 @@ public void NotificationWriterDisposeShouldInvokeStreamDisposed(bool synchronous
public void NotificationWriterDisposeShouldBeIdempotent(bool synchronous, string expected)
{
var notificationWriter = new ODataNotificationWriter(
new StreamWriter(new MemoryStream()),
this.writer,
this.streamListener,
synchronous);

Expand All @@ -69,7 +69,7 @@ public void NotificationWriterDisposeShouldBeIdempotent(bool synchronous, string
public async Task NotificationWriterDisposeShouldInvokeStreamDisposedAsync()
{
await using (var notificationWriter = new ODataNotificationWriter(
new StreamWriter(new MemoryStream()),
this.writer,
this.streamListener)) // `synchronous` argument becomes irrelevant since we'll directly call DisposeAsync
{
}
Expand All @@ -83,7 +83,7 @@ public async Task NotificationWriterDisposeShouldInvokeStreamDisposedAsync()
public async Task NotificationWriterDisposeAsyncShouldBeIdempotent()
{
var notificationWriter = new ODataNotificationWriter(
new StreamWriter(new MemoryStream()),
this.writer,
this.streamListener);

// 1st call to DisposeAsync
Expand All @@ -102,7 +102,7 @@ public async Task NotificationWriterDisposeAsyncShouldBeIdempotent()
public async Task NotificationWriterDisposeShouldInvokeStreamDisposedAsync()
{
using (var notificationWriter = new ODataNotificationWriter(
new StreamWriter(new MemoryStream()),
this.writer,
this.streamListener,
/*synchronous*/ false))
{
Expand Down