From fa99177e3cb2411655d86597300c4484fd9d368e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Oct 2025 01:46:09 +0000 Subject: [PATCH 1/4] Initial plan From 359de6bfae00dcccc12086a2ba643ab36d7bd7f6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Oct 2025 01:52:46 +0000 Subject: [PATCH 2/4] Fix infinite loop in RequestSocket SendMultipartMessage Update Mailbox.TryRecv to only catch SocketException instead of all exceptions. This allows ObjectDisposedException to propagate and break out of the TrySend loop when a socket is disposed during a send operation. Add test to validate that using a disposed socket doesn't cause an infinite loop. Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com> --- src/NetMQ.Tests/ReqRepTests.cs | 59 +++++++++++++++++++++++++++++++++- src/NetMQ/Core/Mailbox.cs | 2 +- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/NetMQ.Tests/ReqRepTests.cs b/src/NetMQ.Tests/ReqRepTests.cs index 37afa51b1..e6df5008d 100644 --- a/src/NetMQ.Tests/ReqRepTests.cs +++ b/src/NetMQ.Tests/ReqRepTests.cs @@ -1,4 +1,7 @@ -using NetMQ.Sockets; +using System; +using System.Threading; +using System.Threading.Tasks; +using NetMQ.Sockets; using Xunit; namespace NetMQ.Tests @@ -282,5 +285,59 @@ public void SendMultipartMessageSucceeds() Assert.Equal(new[] { "Hello", "Back" }, req.ReceiveMultipartStrings()); } } + + [Fact] + public async Task DisposedSocketDoesNotCauseInfiniteLoop() + { + // This test validates that when a socket is disposed while a send operation is in progress, + // it doesn't cause an infinite loop in TrySend. The fix ensures that Mailbox.TryRecv only + // catches SocketException and not ObjectDisposedException, allowing the exception to propagate + // and break out of the sending loop. + + var req = new RequestSocket(); + req.Options.Linger = TimeSpan.Zero; + + // Bind to an endpoint that has no peer - this will cause sends to block + req.Connect("tcp://localhost:15555"); + + // Dispose the socket to trigger ObjectDisposedException + req.Dispose(); + + // Create a task that will attempt to send on the disposed socket + // This should complete quickly by throwing an exception rather than hanging + var sendTask = Task.Run(() => + { + try + { + var msg = new NetMQMessage(); + msg.Append("test"); + req.SendMultipartMessage(msg); + } + catch (ObjectDisposedException) + { + // Expected exception - this is good, it means we broke out of the loop + return true; + } + catch (Exception) + { + // Any other exception is also fine - the key is that we don't hang + return true; + } + return false; + }); + + // If the fix is working, this should complete quickly (within 5 seconds) + // If there's an infinite loop, it will timeout + var timeoutTask = Task.Delay(TimeSpan.FromSeconds(5)); + var completedTask = await Task.WhenAny(sendTask, timeoutTask); + + Assert.True(completedTask == sendTask, "Send operation should complete quickly after disposal, not hang in infinite loop"); + + if (completedTask == sendTask) + { + var result = await sendTask; + Assert.True(result, "Send operation should have thrown an exception"); + } + } } } diff --git a/src/NetMQ/Core/Mailbox.cs b/src/NetMQ/Core/Mailbox.cs index e958ec8d9..ac9226506 100644 --- a/src/NetMQ/Core/Mailbox.cs +++ b/src/NetMQ/Core/Mailbox.cs @@ -213,7 +213,7 @@ public bool TryRecv(int timeout, out Command command) m_active = false; m_signaler.Recv(); } - catch + catch (SocketException) { m_active = true; command = default(Command); From 933c7f6b3ee49870dc58c44dcabcdc16645daf47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Oct 2025 02:32:19 +0000 Subject: [PATCH 3/4] Remove catch-all exception handler from test Per review feedback, remove the general exception catch block from the test. If an unexpected exception is thrown, the task will fault and the test will fail when awaited, which is the desired behavior. Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com> --- src/NetMQ.Tests/ReqRepTests.cs | 29 +++++------------------------ 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/src/NetMQ.Tests/ReqRepTests.cs b/src/NetMQ.Tests/ReqRepTests.cs index e6df5008d..888b9ad07 100644 --- a/src/NetMQ.Tests/ReqRepTests.cs +++ b/src/NetMQ.Tests/ReqRepTests.cs @@ -297,7 +297,7 @@ public async Task DisposedSocketDoesNotCauseInfiniteLoop() var req = new RequestSocket(); req.Options.Linger = TimeSpan.Zero; - // Bind to an endpoint that has no peer - this will cause sends to block + // Connect to an endpoint that has no peer - this will cause sends to block req.Connect("tcp://localhost:15555"); // Dispose the socket to trigger ObjectDisposedException @@ -307,37 +307,18 @@ public async Task DisposedSocketDoesNotCauseInfiniteLoop() // This should complete quickly by throwing an exception rather than hanging var sendTask = Task.Run(() => { - try - { - var msg = new NetMQMessage(); - msg.Append("test"); - req.SendMultipartMessage(msg); - } - catch (ObjectDisposedException) - { - // Expected exception - this is good, it means we broke out of the loop - return true; - } - catch (Exception) - { - // Any other exception is also fine - the key is that we don't hang - return true; - } - return false; + var msg = new NetMQMessage(); + msg.Append("test"); + req.SendMultipartMessage(msg); }); // If the fix is working, this should complete quickly (within 5 seconds) // If there's an infinite loop, it will timeout + // The task may complete with an exception (which is fine - we just want it to complete) var timeoutTask = Task.Delay(TimeSpan.FromSeconds(5)); var completedTask = await Task.WhenAny(sendTask, timeoutTask); Assert.True(completedTask == sendTask, "Send operation should complete quickly after disposal, not hang in infinite loop"); - - if (completedTask == sendTask) - { - var result = await sendTask; - Assert.True(result, "Send operation should have thrown an exception"); - } } } } From a88079c06f7c06a478c800cc54486b0bcfb60c47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Oct 2025 03:00:54 +0000 Subject: [PATCH 4/4] Remove test that doesn't reliably reproduce the race condition The test for DisposedSocketDoesNotCauseInfiniteLoop doesn't reliably reproduce the specific race condition fixed in Mailbox.TryRecv (disposal during send with m_active=true). The fix (catching only SocketException instead of all exceptions) is straightforward and best verified through code review of Mailbox.TryRecv. Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com> --- src/NetMQ.Tests/ReqRepTests.cs | 40 +--------------------------------- 1 file changed, 1 insertion(+), 39 deletions(-) diff --git a/src/NetMQ.Tests/ReqRepTests.cs b/src/NetMQ.Tests/ReqRepTests.cs index 888b9ad07..37afa51b1 100644 --- a/src/NetMQ.Tests/ReqRepTests.cs +++ b/src/NetMQ.Tests/ReqRepTests.cs @@ -1,7 +1,4 @@ -using System; -using System.Threading; -using System.Threading.Tasks; -using NetMQ.Sockets; +using NetMQ.Sockets; using Xunit; namespace NetMQ.Tests @@ -285,40 +282,5 @@ public void SendMultipartMessageSucceeds() Assert.Equal(new[] { "Hello", "Back" }, req.ReceiveMultipartStrings()); } } - - [Fact] - public async Task DisposedSocketDoesNotCauseInfiniteLoop() - { - // This test validates that when a socket is disposed while a send operation is in progress, - // it doesn't cause an infinite loop in TrySend. The fix ensures that Mailbox.TryRecv only - // catches SocketException and not ObjectDisposedException, allowing the exception to propagate - // and break out of the sending loop. - - var req = new RequestSocket(); - req.Options.Linger = TimeSpan.Zero; - - // Connect to an endpoint that has no peer - this will cause sends to block - req.Connect("tcp://localhost:15555"); - - // Dispose the socket to trigger ObjectDisposedException - req.Dispose(); - - // Create a task that will attempt to send on the disposed socket - // This should complete quickly by throwing an exception rather than hanging - var sendTask = Task.Run(() => - { - var msg = new NetMQMessage(); - msg.Append("test"); - req.SendMultipartMessage(msg); - }); - - // If the fix is working, this should complete quickly (within 5 seconds) - // If there's an infinite loop, it will timeout - // The task may complete with an exception (which is fine - we just want it to complete) - var timeoutTask = Task.Delay(TimeSpan.FromSeconds(5)); - var completedTask = await Task.WhenAny(sendTask, timeoutTask); - - Assert.True(completedTask == sendTask, "Send operation should complete quickly after disposal, not hang in infinite loop"); - } } }