|
| 1 | +commit f656af6b796a9ee5e103c38cafd2346b95fdc759 |
| 2 | +Author: Johannes Weiss < [email protected]> |
| 3 | +Date: Thu Aug 28 13:08:45 2025 +0100 |
| 4 | + |
| 5 | + SelectableEventLoop.debugDescription: fix debugDescription deadlock (#3360) |
| 6 | + |
| 7 | + ### Motivation: |
| 8 | + |
| 9 | + In #3297, I introduced a deadlock involving `SelectableEventLoop`'s |
| 10 | + `debugDescription`. I was under the impression that it's not possible to |
| 11 | + call this from outside the `NIO` module so I deemed it safe. That was a |
| 12 | + mistake :). |
| 13 | + |
| 14 | + ### Modifications: |
| 15 | + |
| 16 | + - Make it impossible to deadlock around |
| 17 | + `SelectableEventLoop.debugDescription`. |
| 18 | + |
| 19 | + ### Result: |
| 20 | + |
| 21 | + - Fewer deadlocks |
| 22 | + |
| 23 | +diff --git a/Sources/NIOPosix/SelectableEventLoop.swift b/Sources/NIOPosix/SelectableEventLoop.swift |
| 24 | +index 6ca89bd3..e696d505 100644 |
| 25 | +--- a/Sources/NIOPosix/SelectableEventLoop.swift |
| 26 | ++++ b/Sources/NIOPosix/SelectableEventLoop.swift |
| 27 | +@@ -1060,28 +1060,31 @@ extension SelectableEventLoop: CustomStringConvertible, CustomDebugStringConvert |
| 28 | + |
| 29 | + @usableFromInline |
| 30 | + var debugDescription: String { |
| 31 | +- self._tasksLock.withLock { |
| 32 | +- if self.inEventLoop { |
| 33 | +- return """ |
| 34 | +- SelectableEventLoop { \ |
| 35 | +- selector = \(self._selector), \ |
| 36 | +- scheduledTasks = \(self._scheduledTasks.description), \ |
| 37 | +- thread = \(self.thread), \ |
| 38 | +- state = \(self.internalState) \ |
| 39 | +- } |
| 40 | +- """ |
| 41 | +- } else { |
| 42 | +- return self.externalStateLock.withLock { |
| 43 | +- """ |
| 44 | +- SelectableEventLoop { \ |
| 45 | +- selector = \(self._selector), \ |
| 46 | +- scheduledTasks = \(self._scheduledTasks.description), \ |
| 47 | +- thread = \(self.thread), \ |
| 48 | +- state = \(self.externalState) \ |
| 49 | +- } |
| 50 | +- """ |
| 51 | ++ let scheduledTasks = self._tasksLock.withLock { |
| 52 | ++ self._scheduledTasks.description |
| 53 | ++ } |
| 54 | ++ |
| 55 | ++ if self.inEventLoop { |
| 56 | ++ return """ |
| 57 | ++ SelectableEventLoop { \ |
| 58 | ++ selector = \(self._selector), \ |
| 59 | ++ scheduledTasks = \(scheduledTasks), \ |
| 60 | ++ thread = \(self.thread), \ |
| 61 | ++ state = \(self.internalState) \ |
| 62 | + } |
| 63 | ++ """ |
| 64 | ++ } else { |
| 65 | ++ let externalState = self.externalStateLock.withLock { |
| 66 | ++ self.externalState |
| 67 | + } |
| 68 | ++ return """ |
| 69 | ++ SelectableEventLoop { \ |
| 70 | ++ selector = \(self._selector), \ |
| 71 | ++ scheduledTasks = \(scheduledTasks), \ |
| 72 | ++ thread = \(self.thread), \ |
| 73 | ++ state = \(externalState) \ |
| 74 | ++ } |
| 75 | ++ """ |
| 76 | + } |
| 77 | + } |
| 78 | + } |
| 79 | +diff --git a/Tests/NIOPosixTests/EventLoopTest.swift b/Tests/NIOPosixTests/EventLoopTest.swift |
| 80 | +index 65bbc653..93a5a700 100644 |
| 81 | +--- a/Tests/NIOPosixTests/EventLoopTest.swift |
| 82 | ++++ b/Tests/NIOPosixTests/EventLoopTest.swift |
| 83 | +@@ -2070,6 +2070,40 @@ final class EventLoopTest: XCTestCase { |
| 84 | + XCTAssertEqual("cool", actual) |
| 85 | + } |
| 86 | + #endif |
| 87 | ++ |
| 88 | ++ func testRegressionSelectableEventLoopDeadlock() throws { |
| 89 | ++ let iterations = 1_000 |
| 90 | ++ let loop = MultiThreadedEventLoopGroup.singleton.next() as! SelectableEventLoop |
| 91 | ++ let threadsReadySem = DispatchSemaphore(value: 0) |
| 92 | ++ let go = DispatchSemaphore(value: 0) |
| 93 | ++ |
| 94 | ++ let scheduleds = NIOThreadPool.singleton.runIfActive(eventLoop: loop) { |
| 95 | ++ threadsReadySem.signal() |
| 96 | ++ go.wait() |
| 97 | ++ var tasks: [Scheduled<()>] = [] |
| 98 | ++ for _ in 0..<iterations { |
| 99 | ++ tasks.append(loop.scheduleTask(in: .milliseconds(1)) {}) |
| 100 | ++ } |
| 101 | ++ return tasks |
| 102 | ++ } |
| 103 | ++ |
| 104 | ++ let descriptions = NIOThreadPool.singleton.runIfActive(eventLoop: loop) { |
| 105 | ++ threadsReadySem.signal() |
| 106 | ++ go.wait() |
| 107 | ++ var descriptions: [String] = [] |
| 108 | ++ for _ in 0..<iterations { |
| 109 | ++ descriptions.append(loop.debugDescription) |
| 110 | ++ } |
| 111 | ++ return descriptions |
| 112 | ++ } |
| 113 | ++ |
| 114 | ++ threadsReadySem.wait() |
| 115 | ++ threadsReadySem.wait() |
| 116 | ++ go.signal() |
| 117 | ++ go.signal() |
| 118 | ++ XCTAssertEqual(iterations, try scheduleds.wait().map { $0.cancel() }.count) |
| 119 | ++ XCTAssertEqual(iterations, try descriptions.wait().count) |
| 120 | ++ } |
| 121 | + } |
| 122 | + |
| 123 | + private final class EventLoopWithPreSucceededFuture: EventLoop { |
0 commit comments