Skip to content

PipeBase<TMessage> should implement the Dispose Pattern correctly #930

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

Jump to bottom
Closed
eerhardt opened this issue Sep 17, 2018 · 0 comments
Closed

PipeBase<TMessage> should implement the Dispose Pattern correctly #930

eerhardt opened this issue Sep 17, 2018 · 0 comments
Labels
up-for-grabs A good issue to fix if you are trying to contribute to the project

Comments

@eerhardt
Copy link
Member

The PipeBase class implements IDisposable, but doesn't implement the basic pattern correctly.

machinelearning/src/Microsoft.ML.Core/Environment/HostEnvironmentBase.cs

Lines 205 to 213 in 5e08fa1

public void Dispose()
{
DisposeCore();
}
protected virtual void DisposeCore()
{
IsActive = false;
}

See https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern for the basic dispose pattern.

We should follow the design guidelines here.
@eerhardt eerhardt mentioned this issue Sep 17, 2018
Merged
@Ivanidzo4ka Ivanidzo4ka added the up-for-grabs A good issue to fix if you are trying to contribute to the project label Oct 19, 2018
@feiyun0112 feiyun0112 mentioned this issue Oct 28, 2018
Merged
Zruty0 pushed a commit that referenced this issue Oct 29, 2018
@feiyun0112 @Zruty0
PipeBase<TMessage> implement the Dispose Pattern (#1418)
4b83195 
Fixes #930
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
up-for-grabs A good issue to fix if you are trying to contribute to the project
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Ivanidzo4ka @eerhardt