From 69fb3be4fa27c01d737a21c2835479685745453d Mon Sep 17 00:00:00 2001 From: Gert Driesen Date: Sat, 30 Sep 2017 21:42:56 +0200 Subject: [PATCH] Signal the closed wait handle to avoid a deadlock when a subscriber to the Closed event in turn closes or disposes the channel. --- ...hannelEofReceived_DisposeInEventHandler.cs | 136 ++++++++++++++++++ .../Renci.SshNet.Tests.csproj | 1 + src/Renci.SshNet/Channels/Channel.cs | 13 +- 3 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 src/Renci.SshNet.Tests/Classes/Channels/ChannelSessionTest_Dispose_SessionIsConnectedAndChannelIsOpen_ChannelCloseAndChannelEofReceived_DisposeInEventHandler.cs diff --git a/src/Renci.SshNet.Tests/Classes/Channels/ChannelSessionTest_Dispose_SessionIsConnectedAndChannelIsOpen_ChannelCloseAndChannelEofReceived_DisposeInEventHandler.cs b/src/Renci.SshNet.Tests/Classes/Channels/ChannelSessionTest_Dispose_SessionIsConnectedAndChannelIsOpen_ChannelCloseAndChannelEofReceived_DisposeInEventHandler.cs new file mode 100644 index 000000000..35e70ac3d --- /dev/null +++ b/src/Renci.SshNet.Tests/Classes/Channels/ChannelSessionTest_Dispose_SessionIsConnectedAndChannelIsOpen_ChannelCloseAndChannelEofReceived_DisposeInEventHandler.cs @@ -0,0 +1,136 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using Renci.SshNet.Channels; +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Connection; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes.Channels +{ + [TestClass] + public class ChannelSessionTest_Dispose_SessionIsConnectedAndChannelIsOpen_ChannelCloseAndChannelEofReceived_DisposeInEventHandler + { + private Mock _sessionMock; + private uint _localChannelNumber; + private uint _localWindowSize; + private uint _localPacketSize; + private uint _remoteChannelNumber; + private uint _remoteWindowSize; + private uint _remotePacketSize; + private IList _channelClosedRegister; + private List _channelExceptionRegister; + private ChannelSession _channel; + private Mock _connectionInfoMock; + private MockSequence _sequence; + private SemaphoreLight _sessionSemaphore; + private int _initialSessionSemaphoreCount; + + [TestInitialize] + public void Initialize() + { + Arrange(); + Act(); + } + + private void Arrange() + { + var random = new Random(); + _localChannelNumber = (uint)random.Next(0, int.MaxValue); + _localWindowSize = (uint)random.Next(0, int.MaxValue); + _localPacketSize = (uint)random.Next(0, int.MaxValue); + _remoteChannelNumber = (uint)random.Next(0, int.MaxValue); + _remoteWindowSize = (uint)random.Next(0, int.MaxValue); + _remotePacketSize = (uint)random.Next(0, int.MaxValue); + _channelClosedRegister = new List(); + _channelExceptionRegister = new List(); + _initialSessionSemaphoreCount = random.Next(10, 20); + _sessionSemaphore = new SemaphoreLight(_initialSessionSemaphoreCount); + + _sessionMock = new Mock(MockBehavior.Strict); + _connectionInfoMock = new Mock(MockBehavior.Strict); + + _sequence = new MockSequence(); + _sessionMock.InSequence(_sequence).Setup(p => p.ConnectionInfo).Returns(_connectionInfoMock.Object); + _connectionInfoMock.InSequence(_sequence).Setup(p => p.RetryAttempts).Returns(1); + _sessionMock.Setup(p => p.SessionSemaphore).Returns(_sessionSemaphore); + _sessionMock.InSequence(_sequence) + .Setup( + p => + p.SendMessage( + It.Is( + m => + m.LocalChannelNumber == _localChannelNumber && + m.InitialWindowSize == _localWindowSize && m.MaximumPacketSize == _localPacketSize && + m.Info is SessionChannelOpenInfo))); + _sessionMock.InSequence(_sequence) + .Setup(p => p.WaitOnHandle(It.IsNotNull())) + .Callback( + w => + { + _sessionMock.Raise( + s => s.ChannelOpenConfirmationReceived += null, + new MessageEventArgs( + new ChannelOpenConfirmationMessage( + _localChannelNumber, + _remoteWindowSize, + _remotePacketSize, + _remoteChannelNumber))); + w.WaitOne(); + }); + _sessionMock.InSequence(_sequence).Setup(p => p.IsConnected).Returns(true); + _sessionMock.InSequence(_sequence) + .Setup(p => p.TrySendMessage(It.Is(c => c.LocalChannelNumber == _remoteChannelNumber))) + .Returns(true); + _sessionMock.InSequence(_sequence) + .Setup(s => s.WaitOnHandle(It.IsNotNull())) + .Callback(w => w.WaitOne()); + + _channel = new ChannelSession(_sessionMock.Object, _localChannelNumber, _localWindowSize, _localPacketSize); + _channel.Closed += (sender, args) => + { + _channelClosedRegister.Add(args); + _channel.Dispose(); + }; + _channel.Exception += (sender, args) => _channelExceptionRegister.Add(args); + _channel.Open(); + + _sessionMock.Raise(p => p.ChannelEofReceived += null, + new MessageEventArgs(new ChannelEofMessage(_localChannelNumber))); + _sessionMock.Raise(p => p.ChannelCloseReceived += null, + new MessageEventArgs(new ChannelCloseMessage(_localChannelNumber))); + } + + private void Act() + { + _channel.Dispose(); + } + + [TestMethod] + public void CurrentCountOfSessionSemaphoreShouldBeEqualToInitialCount() + { + Assert.AreEqual(_initialSessionSemaphoreCount, _sessionSemaphore.CurrentCount); + } + + [TestMethod] + public void ExceptionShouldNeverHaveFired() + { + Assert.AreEqual(0, _channelExceptionRegister.Count, _channelExceptionRegister.AsString()); + } + + [TestMethod] + public void ClosedEventShouldHaveFiredOnce() + { + Assert.AreEqual(1, _channelClosedRegister.Count); + Assert.AreEqual(_localChannelNumber, _channelClosedRegister[0].ChannelNumber); + } + + [TestMethod] + public void IsOpenShouldReturnFalse() + { + Assert.IsFalse(_channel.IsOpen); + } + } +} diff --git a/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj b/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj index b34d1bf61..456fdcff6 100644 --- a/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj +++ b/src/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj @@ -94,6 +94,7 @@ + diff --git a/src/Renci.SshNet/Channels/Channel.cs b/src/Renci.SshNet/Channels/Channel.cs index 818f6959d..a5c449bf6 100644 --- a/src/Renci.SshNet/Channels/Channel.cs +++ b/src/Renci.SshNet/Channels/Channel.cs @@ -400,6 +400,14 @@ protected virtual void OnClose() { _closeMessageReceived = true; + // signal that SSH_MSG_CHANNEL_CLOSE message was received from server + // we need to signal this before firing the Closed event, as a subscriber + // may very well react to the Closed event by closing or disposing the + // channel which in turn will wait for this handle to be signaled + var channelClosedWaitHandle = _channelClosedWaitHandle; + if (channelClosedWaitHandle != null) + channelClosedWaitHandle.Set(); + // raise event signaling that the server has closed its end of the channel var closed = Closed; if (closed != null) @@ -407,11 +415,6 @@ protected virtual void OnClose() closed(this, new ChannelEventArgs(LocalChannelNumber)); } - // signal that SSH_MSG_CHANNEL_CLOSE message was received from server - var channelClosedWaitHandle = _channelClosedWaitHandle; - if (channelClosedWaitHandle != null) - channelClosedWaitHandle.Set(); - // close the channel Close(); }