Skip to content

Commit ec234d9

Browse files
alanmcgovernalanmcgovern
authored
Merge pull request #689 from alanmcgovern/ensure-attempted-connections-have-timeouts
Treat peers as 'connecting' until the handshake completes
2 parents e60b1c8 + 303ad5e commit ec234d9

File tree

6 files changed

+148
-24
lines changed

6 files changed

+148
-24
lines changed

‎src/MonoTorrent.Client/MonoTorrent.Client/Managers/ConnectionManager.cs‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -146,12 +146,15 @@ async void ConnectToPeer (TorrentManager manager, Peer peer)
146146
succeeded = false;
147147
}
148148

149-
PendingConnects.Remove (state);
150-
manager.Peers.ConnectingToPeers.Remove (peer);
151149
if (manager.Disposed ||
152150
!manager.Mode.CanAcceptConnections ||
153151
OpenConnections > Settings.MaximumConnections ||
154152
manager.OpenConnections > manager.Settings.MaximumConnections) {
153+
154+
// Remove from pending connects.
155+
PendingConnects.Remove (state);
156+
manager.Peers.ConnectingToPeers.Remove (peer);
157+
155158
manager.Peers.AvailablePeers.Add (peer);
156159
connection.Dispose ();
157160
return;
@@ -165,14 +168,17 @@ async void ConnectToPeer (TorrentManager manager, Peer peer)
165168
} else {
166169
logger.Info (connection, "Connection opened");
167170

168-
// FIXME: We should include handshaking in the connection timeout, but allow more time.
169-
// WE no longer create/add a peerid to the main list *before* handshaking is done, so
170-
// we need a way to cancel stuck connection attempts.
171-
ProcessNewOutgoingConnection (manager, peer, connection);
171+
// Reset the connection timer so there's a little bit of extra time for the handshake.
172+
state.Timer.Restart ();
173+
await ProcessNewOutgoingConnection (manager, peer, connection);
172174
}
173175
} catch {
174176
// FIXME: Do nothing now?
175177
} finally {
178+
// Either we've successfully handshaked, or the attempt failed. We can remove them from these lists now.
179+
PendingConnects.Remove (state);
180+
manager.Peers.ConnectingToPeers.Remove (peer);
181+
176182
// Try to connect to another peer
177183
TryConnect ();
178184
}
@@ -183,7 +189,7 @@ internal bool Contains (TorrentManager manager)
183189
return Torrents.Contains (manager);
184190
}
185191

186-
internal async void ProcessNewOutgoingConnection (TorrentManager manager, Peer peer, IPeerConnection connection)
192+
internal async ReusableTask ProcessNewOutgoingConnection (TorrentManager manager, Peer peer, IPeerConnection connection)
187193
{
188194
var bitfield = new BitField (manager.Bitfield.Length);
189195
// If we have too many open connections, close the connection
@@ -210,7 +216,7 @@ internal async void ProcessNewOutgoingConnection (TorrentManager manager, Peer p
210216
var preferredEncryption = EncryptionTypes.GetPreferredEncryption (peer.AllowedEncryption, Settings.AllowedEncryption);
211217
if (preferredEncryption.Count == 0)
212218
throw new NotSupportedException ("The peer and the engine do not agree on any encryption methods");
213-
EncryptorFactory.EncryptorResult result = await EncryptorFactory.CheckOutgoingConnectionAsync (connection, preferredEncryption, manager.InfoHashes.V1OrV2.Truncate (), handshake, manager.Engine!.Factories);
219+
EncryptorFactory.EncryptorResult result = await EncryptorFactory.CheckOutgoingConnectionAsync (connection, preferredEncryption, manager.InfoHashes.V1OrV2.Truncate (), handshake, manager.Engine!.Factories, Settings.ConnectionTimeout);
214220
decryptor = result.Decryptor;
215221
encryptor = result.Encryptor;
216222
} catch {
@@ -561,17 +567,14 @@ internal void TryConnect ()
561567
bool TryConnect (TorrentManager manager)
562568
{
563569
int i;
570+
// If the torrent isn't active, don't connect to a peer for it
564571
if (!manager.Mode.CanAcceptConnections)
565572
return false;
566573

567574
// If we have reached the max peers allowed for this torrent, don't connect to a new peer for this torrent
568575
if ((manager.Peers.ConnectedPeers.Count + manager.Peers.ConnectingToPeers.Count) >= manager.Settings.MaximumConnections)
569576
return false;
570577

571-
// If the torrent isn't active, don't connect to a peer for it
572-
if (!manager.Mode.CanAcceptConnections)
573-
return false;
574-
575578
// If we are not seeding, we can connect to anyone. If we are seeding, we should only connect to a peer
576579
// if they are not a seeder.
577580
for (i = 0; i < manager.Peers.AvailablePeers.Count; i++)

‎src/MonoTorrent.Client/MonoTorrent.Client/Managers/ListenManager.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,14 +100,14 @@ async void ConnectionReceived (object? sender, PeerConnectionEventArgs e)
100100
if (!e.Connection.IsIncoming) {
101101
var manager = Engine.Torrents.First (t => t.InfoHashes.Contains (e.InfoHash!));
102102
var peer = new Peer (peerInfo);
103-
Engine.ConnectionManager.ProcessNewOutgoingConnection (manager, peer, e.Connection);
103+
await Engine.ConnectionManager.ProcessNewOutgoingConnection (manager, peer, e.Connection);
104104
return;
105105
}
106106

107107
logger.Info (e.Connection, "ConnectionReceived");
108108

109109
var supportedEncryptions = Engine.Settings.AllowedEncryption;
110-
EncryptorFactory.EncryptorResult result = await EncryptorFactory.CheckIncomingConnectionAsync (e.Connection, supportedEncryptions, SKeys, Engine.Factories);
110+
EncryptorFactory.EncryptorResult result = await EncryptorFactory.CheckIncomingConnectionAsync (e.Connection, supportedEncryptions, SKeys, Engine.Factories, Engine.Settings.ConnectionTimeout);
111111
if (!await HandleHandshake (peerInfo, e.Connection, result.Handshake!, result.Decryptor, result.Encryptor))
112112
e.Connection.Dispose ();
113113
} catch (Exception ex) {

‎src/MonoTorrent.Client/MonoTorrent.Connections.Peer.Encryption/EncryptorFactory.cs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,12 @@ public EncryptorResult (IEncryption decryptor, IEncryption encryptor, HandshakeM
5555
}
5656
}
5757

58-
static TimeSpan Timeout => Debugger.IsAttached ? TimeSpan.FromHours (1) : TimeSpan.FromSeconds (10);
59-
60-
internal static async ReusableTask<EncryptorResult> CheckIncomingConnectionAsync (IPeerConnection connection, IList<EncryptionType> allowedEncryption, InfoHash[] sKeys, Factories factories)
58+
internal static async ReusableTask<EncryptorResult> CheckIncomingConnectionAsync (IPeerConnection connection, IList<EncryptionType> allowedEncryption, InfoHash[] sKeys, Factories factories, TimeSpan timeout)
6159
{
6260
if (!connection.IsIncoming)
6361
throw new Exception ("oops");
6462

65-
using var cts = new CancellationTokenSource (Timeout);
63+
using var cts = new CancellationTokenSource (timeout);
6664
using CancellationTokenRegistration registration = cts.Token.Register (connection.Dispose);
6765
return await DoCheckIncomingConnectionAsync (connection, allowedEncryption, sKeys, factories).ConfigureAwait (false);
6866
}
@@ -114,12 +112,12 @@ static async ReusableTask<EncryptorResult> DoCheckIncomingConnectionAsync (IPeer
114112
throw new EncryptionException ("Invalid handshake received and no decryption works");
115113
}
116114

117-
internal static async ReusableTask<EncryptorResult> CheckOutgoingConnectionAsync (IPeerConnection connection, IList<EncryptionType> allowedEncryption, InfoHash infoHash, HandshakeMessage handshake, Factories factories)
115+
internal static async ReusableTask<EncryptorResult> CheckOutgoingConnectionAsync (IPeerConnection connection, IList<EncryptionType> allowedEncryption, InfoHash infoHash, HandshakeMessage handshake, Factories factories, TimeSpan timeout)
118116
{
119117
if (connection.IsIncoming)
120118
throw new Exception ("oops");
121119

122-
using var cts = new CancellationTokenSource (Timeout);
120+
using var cts = new CancellationTokenSource (timeout);
123121
using CancellationTokenRegistration registration = cts.Token.Register (connection.Dispose);
124122
return await DoCheckOutgoingConnectionAsync (connection, allowedEncryption, infoHash, handshake, factories).ConfigureAwait (false);
125123
}

‎src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client.Modes/MetadataModeTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public async Task Setup (bool metadataMode, bool multiFile = false, bool metadat
7171
var connection = pair.Incoming;
7272
PeerId id = new PeerId (new Peer (new PeerInfo (connection.Uri)), connection, new BitField (rig.Torrent.PieceCount), rig.Manager.InfoHashes.V1OrV2);
7373

74-
var result = await EncryptorFactory.CheckIncomingConnectionAsync (id.Connection, id.Peer.AllowedEncryption, new[] { rig.Manager.InfoHashes.V1OrV2 }, Factories.Default);
74+
var result = await EncryptorFactory.CheckIncomingConnectionAsync (id.Connection, id.Peer.AllowedEncryption, new[] { rig.Manager.InfoHashes.V1OrV2 }, Factories.Default, TaskExtensions.Timeout);
7575
decryptor = id.Decryptor = result.Decryptor;
7676
encryptor = id.Encryptor = result.Encryptor;
7777
}

‎src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/ConnectionManagerTests.cs‎

Lines changed: 124 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
1-
using System.Linq;
1+
using System;
2+
using System.Linq;
3+
using System.Net;
4+
using System.Net.Sockets;
25
using System.Threading.Tasks;
36

7+
using MonoTorrent.Client.Modes;
8+
using MonoTorrent.Connections;
9+
using MonoTorrent.Connections.Peer;
10+
using MonoTorrent.Messages.Peer;
11+
412
using NUnit.Framework;
513

14+
using ReusableTasks;
15+
616
namespace MonoTorrent.Client
717
{
818
[TestFixture]
@@ -33,5 +43,118 @@ await engine.AddAsync (new MagnetLink (new InfoHash (Enumerable.Repeat ((byte) 2
3343
Assert.AreEqual (torrents[2], manager.Torrents[1]);
3444
Assert.AreEqual (torrents[0], manager.Torrents[2]);
3545
}
46+
47+
class FakeConnection : IPeerConnection
48+
{
49+
public ReadOnlyMemory<byte> AddressBytes { get; }
50+
public bool CanReconnect { get; }
51+
public bool Disposed { get; private set; }
52+
public IPEndPoint EndPoint { get; }
53+
public bool IsIncoming { get; }
54+
public Uri Uri { get; }
55+
56+
public FakeConnection (Uri uri)
57+
=> Uri = uri;
58+
59+
public ReusableTaskCompletionSource<bool> ConnectAsyncInvokedTask = new ReusableTaskCompletionSource<bool> ();
60+
public ReusableTaskCompletionSource<bool> ConnectAsyncResultTask = new ReusableTaskCompletionSource<bool> ();
61+
public async ReusableTask ConnectAsync ()
62+
{
63+
ConnectAsyncInvokedTask.SetResult (true);
64+
await ConnectAsyncResultTask.Task;
65+
}
66+
67+
public TaskCompletionSource<bool> DisposeAsyncInvokedTask = new TaskCompletionSource<bool> ();
68+
public void Dispose ()
69+
{
70+
TestContext.Out.WriteLine (Environment.StackTrace);
71+
Disposed = true;
72+
ConnectAsyncResultTask.SetException (new SocketException ((int) SocketError.ConnectionAborted));
73+
DisposeAsyncInvokedTask.SetResult (true);
74+
}
75+
76+
public ReusableTaskCompletionSource<Memory<byte>> ReceiveAsyncInvokedTask = new ReusableTaskCompletionSource<Memory<byte>> ();
77+
public ReusableTaskCompletionSource<int> ReceiveAsyncResultTask = new ReusableTaskCompletionSource<int> ();
78+
public async ReusableTask<int> ReceiveAsync (Memory<byte> buffer)
79+
{
80+
ReceiveAsyncInvokedTask.SetResult (buffer);
81+
return await ReceiveAsyncResultTask.Task;
82+
}
83+
84+
public ReusableTaskCompletionSource<Memory<byte>> SendAsyncInvokedTask = new ReusableTaskCompletionSource<Memory<byte>> ();
85+
public ReusableTaskCompletionSource<int> SendAsyncResultTask = new ReusableTaskCompletionSource<int> ();
86+
public async ReusableTask<int> SendAsync (Memory<byte> buffer)
87+
{
88+
SendAsyncInvokedTask.SetResult (buffer);
89+
return await SendAsyncResultTask.Task;
90+
}
91+
}
92+
93+
[Test]
94+
public async Task CancelPending_WaitingForConnect ()
95+
{
96+
var fake = new FakeConnection (new Uri ("ipv4://1.2.3.4:56789"));
97+
var engine = EngineHelpers.Create (
98+
EngineHelpers.CreateSettings (),
99+
EngineHelpers.Factories.WithPeerConnectionCreator ("ipv4", t => fake)
100+
);
101+
102+
var connectionManager = new ConnectionManager ("test", engine.Settings, engine.Factories, engine.DiskManager);
103+
104+
var manager = await engine.AddAsync (new MagnetLink (new InfoHash (Enumerable.Repeat ((byte) 0, 20).ToArray ())), "tmp");
105+
manager.Mode = new MetadataMode (manager, engine.DiskManager, connectionManager, engine.Settings, "");
106+
manager.Peers.AvailablePeers.Add (PeerId.CreateNull (1, manager.InfoHashes.V1OrV2).Peer);
107+
connectionManager.Add (manager);
108+
109+
await ClientEngine.MainLoop;
110+
111+
// Initiate a connection
112+
connectionManager.TryConnect ();
113+
await fake.ConnectAsyncInvokedTask.Task.WithTimeout ();
114+
115+
// Abort it while we're waiting for the connection to succeed.
116+
connectionManager.CancelPendingConnects (manager);
117+
118+
// Make sure the connection was disposed.
119+
await fake.DisposeAsyncInvokedTask.Task.WithTimeout ();
120+
Assert.IsTrue (fake.Disposed);
121+
}
122+
123+
[Test]
124+
public async Task CancelPending_SendingHandshake ()
125+
{
126+
var fake = new FakeConnection (new Uri ("ipv4://1.2.3.4:56789"));
127+
var builder = new EngineSettingsBuilder (EngineHelpers.CreateSettings ());
128+
builder.ConnectionTimeout = TimeSpan.FromHours (1);
129+
var engine = EngineHelpers.Create (
130+
builder.ToSettings (),
131+
EngineHelpers.Factories.WithPeerConnectionCreator ("ipv4",t => {
132+
return fake;
133+
})
134+
);
135+
136+
var connectionManager = new ConnectionManager ("test", engine.Settings, engine.Factories, engine.DiskManager);
137+
138+
var manager = await engine.AddAsync (new MagnetLink (new InfoHash (Enumerable.Repeat ((byte) 0, 20).ToArray ())), "tmp");
139+
manager.Mode = new MetadataMode (manager, engine.DiskManager, connectionManager, engine.Settings, "");
140+
manager.Peers.AvailablePeers.Add (new Peer (new PeerInfo (fake.Uri), new[] { EncryptionType.PlainText }));
141+
connectionManager.Add (manager);
142+
143+
await ClientEngine.MainLoop;
144+
145+
// Initiate a connection and allow it to succeed
146+
connectionManager.TryConnect ();
147+
await fake.ConnectAsyncInvokedTask.Task.WithTimeout ();
148+
fake.ConnectAsyncResultTask.SetResult (true);
149+
150+
// Handshake should be sent.
151+
var data = await fake.SendAsyncInvokedTask.Task.WithTimeout ();
152+
var message = new HandshakeMessage (data.Span);
153+
Assert.AreEqual (message.ProtocolString, Constants.ProtocolStringV100);
154+
155+
connectionManager.CancelPendingConnects (manager);
156+
await fake.DisposeAsyncInvokedTask.Task.WithTimeout ();
157+
Assert.IsTrue (fake.Disposed);
158+
}
36159
}
37160
}

‎src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/EncryptorFactoryTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,8 @@ async Task Handshake (IList<EncryptionType> outgoingEncryption, IList<Encryption
166166
var handshakeIn = new HandshakeMessage (InfoHash, IncomingId, Constants.ProtocolStringV100);
167167
var handshakeOut = new HandshakeMessage (InfoHash, OutgoingId, Constants.ProtocolStringV100);
168168

169-
var incomingTask = EncryptorFactory.CheckIncomingConnectionAsync (Incoming, incomingEncryption, SKeys, Factories.Default);
170-
var outgoingTask = EncryptorFactory.CheckOutgoingConnectionAsync (Outgoing, outgoingEncryption, InfoHash, appendInitialPayload ? handshakeOut : null, Factories.Default);
169+
var incomingTask = EncryptorFactory.CheckIncomingConnectionAsync (Incoming, incomingEncryption, SKeys, Factories.Default, TaskExtensions.Timeout);
170+
var outgoingTask = EncryptorFactory.CheckOutgoingConnectionAsync (Outgoing, outgoingEncryption, InfoHash, appendInitialPayload ? handshakeOut : null, Factories.Default, TaskExtensions.Timeout);
171171

172172
// If the handshake was not part of the initial payload, send it now.
173173
var outgoingCrypto = await outgoingTask;

0 commit comments

Comments
 (0)