★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/7746Nouvelle recherche | Portail wanayoo
Skip to content

/PowerShell

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

Ensure the SSHClientSessionTransportManager stream writer and reader fields are cleared after dispose. #7746

Merged
merged 1 commit into from Sep 10, 2018
+4 −5

Conversation

Reviewers

@daxian-dbw daxian-dbw

@PaulHigin PaulHigin

@iSazonov iSazonov

@TravisEz13 TravisEz13

@adityapatwardhan adityapatwardhan

Assignees

@daxian-dbw daxian-dbw

Labels
None yet
Projects
None yet
Milestone
  v6.1.0
5 participants
@dantraMSFT @iSazonov @daxian-dbw @PaulHigin @TravisEz13
@dantraMSFT
Copy link
Contributor

commented Sep 9, 2018

Fix #7740

PR Summary

Fix the ObjectDisposedException in SSHClientSessionTransportManager.CloseConnection calling StreamWriter.Dispose multiple times.

There are two changes:

  • Clear the streamreader and streamwriter fields after disposing.
  • Use InterlockedExchange for the above fields and _sshProcessId to avoid possible race conditions for async usage.

PR Checklist
Ubuntu
Ensure stream writer and reader fields are cleared after dispose. 
Fix possible race conditions accessing the fields during CloseConnection.
Loading status checks…
1a92869

@dantraMSFT dantraMSFT requested review from mirichmo and PaulHigin as code owners Sep 9, 2018

@dantraMSFT dantraMSFT requested review from daxian-dbw, TravisEz13, adityapatwardhan and PaulHigin and removed request for PaulHigin and mirichmo Sep 9, 2018

@iSazonov iSazonov assigned daxian-dbw Sep 10, 2018

@iSazonov

iSazonov approved these changes Sep 10, 2018
View changes

@PaulHigin

PaulHigin reviewed Sep 10, 2018
View changes

Copy link
Contributor

left a comment

LGTM, but this shouldn't be necessary. From Microsoft documentation:
"If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times. Instance methods other than Dispose can throw an ObjectDisposedException when resources are already disposed."
@PaulHigin

PaulHigin reviewed Sep 10, 2018
View changes

Copy link
Contributor

left a comment

Adding link...
https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?view=netframework-4.7.2
@PaulHigin

PaulHigin approved these changes Sep 10, 2018
View changes

@daxian-dbw

daxian-dbw approved these changes Sep 10, 2018
View changes

Copy link
Member

left a comment

LGTM
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Sep 10, 2018

@PaulHigin I guess that Dispose() should support a protection
https://blogs.msdn.microsoft.com/blambert/2009/07/24/a-simple-and-totally-thread-safe-implementation-of-idisposable/

@TravisEz13 TravisEz13 merged commit 528d970 into PowerShell:master Sep 10, 2018
8 checks passed

8 checks passed

CodeFactor No issues found.
Details
PowerShell-CI-linux #PR-7746-20180909.01 succeeded
Details
PowerShell-CI-macos #PR-7746-20180909.01 succeeded
Details
PowerShell-CI-windows #PR-7746-20180909.01 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.

TravisEz13 added a commit that referenced this pull request Sep 10, 2018

@dantraMSFT @TravisEz13
Ensure the `SSHClientSessionTransportManager` stream writer and reade… 
…r fields are cleared after dispose. (#7746)

Fix possible race conditions accessing the fields during CloseConnection.
Loading status checks…
ccee14d

@TravisEz13 TravisEz13 added this to the v6.1.0 milestone Sep 10, 2018

@dantraMSFT dantraMSFT deleted the dantraMSFT:dantra/issue7740 branch Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.