From 69a5600530226f74976be64952426936c804f22e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Mon, 17 Jun 2024 23:19:47 +0200 Subject: [PATCH 1/8] Add missing `error_operation_aborted?` handlers --- src/crystal/system/win32/iocp.cr | 4 ++-- src/crystal/system/win32/socket.cr | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index ba0f11eb2af5..16aa15a04488 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -206,7 +206,7 @@ module Crystal::IOCP operation.wait_for_result(timeout) do |error| case error - when .error_io_incomplete? + when .error_io_incomplete?, .error_operation_aborted? raise IO::TimeoutError.new("#{method} timed out") when .error_handle_eof? return 0_u32 @@ -236,7 +236,7 @@ module Crystal::IOCP operation.wait_for_wsa_result(timeout) do |error| case error - when .wsa_io_incomplete? + when .wsa_io_incomplete?, .error_operation_aborted? raise IO::TimeoutError.new("#{method} timed out") when .wsaeconnreset? return 0_u32 unless connreset_is_error diff --git a/src/crystal/system/win32/socket.cr b/src/crystal/system/win32/socket.cr index 2a540f4df88d..21687cc7f630 100644 --- a/src/crystal/system/win32/socket.cr +++ b/src/crystal/system/win32/socket.cr @@ -216,6 +216,8 @@ module Crystal::System::Socket case error when .wsa_io_incomplete?, .wsaenotsock? return false + when .error_operation_aborted? + raise IO::TimeoutError.new("#{method} timed out") end end From 2174101c49825c41c458a483f85fe735dd59ce0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Mon, 17 Jun 2024 23:19:17 +0200 Subject: [PATCH 2/8] Use regular fiber timeout event --- src/crystal/system/win32/iocp.cr | 39 +++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index 16aa15a04488..bdb3c5036bcb 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -138,8 +138,8 @@ module Crystal::IOCP protected def schedule(&) case @state when .started? - yield @fiber done! + yield @fiber when .cancelled? @@canceled.delete(self) else @@ -161,24 +161,41 @@ module Crystal::IOCP end def done! + @fiber.cancel_timeout @state = :done end + def cancel! : Bool + # Microsoft documentation: + # The application must not free or reuse the OVERLAPPED structure + # associated with the canceled I/O operations until they have completed + # (this does not apply to asynchronous operations that finished + # synchronously, as nothing would be queued to the IOCP) + ret = LibC.CancelIoEx(@handle, self) + if ret.zero? + case error = WinError.value + when .error_not_found? + # Operation has already completed, do nothing + return false + else + raise RuntimeError.from_os_error("CancelIOEx", os_error: error) + end + end + true + end + def wait_for_completion(timeout) if timeout - timeout_event = Crystal::IOCP::Event.new(Fiber.current) - timeout_event.add(timeout) + sleep timeout else - timeout_event = Crystal::IOCP::Event.new(Fiber.current, Time::Span::MAX) + Fiber.suspend end - # memoize event loop to make sure that we still target the same instance - # after wakeup (guaranteed by current MT model but let's be future proof) - event_loop = Crystal::EventLoop.current - event_loop.enqueue(timeout_event) - - Fiber.suspend - event_loop.dequeue(timeout_event) + unless @state.done? + if cancel! + Fiber.suspend + end + end end end From 7e11a5f7ab9e7f4fe77a595efa854cc06e615144 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Tue, 25 Jun 2024 10:34:24 +0200 Subject: [PATCH 3/8] Drop workaround for `overlapped_accept` --- src/crystal/system/win32/iocp.cr | 3 --- src/crystal/system/win32/socket.cr | 6 +----- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index bdb3c5036bcb..3465e86f8b95 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -118,10 +118,7 @@ module Crystal::IOCP def wait_for_wsa_result(timeout, &) wait_for_completion(timeout) - wsa_result { |error| yield error } - end - def wsa_result(&) raise Exception.new("Invalid state #{@state}") unless @state.done? || @state.started? flags = 0_u32 result = LibC.WSAGetOverlappedResult(LibC::SOCKET.new(@handle.address), self, out bytes, false, pointerof(flags)) diff --git a/src/crystal/system/win32/socket.cr b/src/crystal/system/win32/socket.cr index 21687cc7f630..6be2bf04399b 100644 --- a/src/crystal/system/win32/socket.cr +++ b/src/crystal/system/win32/socket.cr @@ -208,11 +208,7 @@ module Crystal::System::Socket return true end - unless operation.wait_for_completion(read_timeout) - raise IO::TimeoutError.new("#{method} timed out") - end - - operation.wsa_result do |error| + operation.wait_for_wsa_result(read_timeout) do |error| case error when .wsa_io_incomplete?, .wsaenotsock? return false From dbe79b5f921081c94eff32bf77811a039da5151e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Tue, 25 Jun 2024 16:11:34 +0200 Subject: [PATCH 4/8] Drop `CANCELLED` state --- src/crystal/system/win32/iocp.cr | 30 +----------------------------- src/crystal/system/win32/socket.cr | 2 -- 2 files changed, 1 insertion(+), 31 deletions(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index 3465e86f8b95..c7a4212ca0b1 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -65,15 +65,11 @@ module Crystal::IOCP enum State STARTED DONE - CANCELLED end @overlapped = LibC::OVERLAPPED.new @fiber = Fiber.current @state : State = :started - property next : OverlappedOperation? - property previous : OverlappedOperation? - @@canceled = Thread::LinkedList(OverlappedOperation).new def initialize(@handle : LibC::HANDLE) end @@ -84,11 +80,7 @@ module Crystal::IOCP def self.run(handle, &) operation = OverlappedOperation.new(handle) - begin - yield operation - ensure - operation.done - end + yield operation end def self.unbox(overlapped : LibC::OVERLAPPED*) @@ -103,8 +95,6 @@ module Crystal::IOCP def wait_for_result(timeout, &) wait_for_completion(timeout) - raise Exception.new("Invalid state #{@state}") unless @state.done? || @state.started? - result = LibC.GetOverlappedResult(@handle, self, out bytes, 0) if result.zero? error = WinError.value @@ -119,7 +109,6 @@ module Crystal::IOCP def wait_for_wsa_result(timeout, &) wait_for_completion(timeout) - raise Exception.new("Invalid state #{@state}") unless @state.done? || @state.started? flags = 0_u32 result = LibC.WSAGetOverlappedResult(LibC::SOCKET.new(@handle.address), self, out bytes, false, pointerof(flags)) if result.zero? @@ -137,26 +126,11 @@ module Crystal::IOCP when .started? done! yield @fiber - when .cancelled? - @@canceled.delete(self) else raise Exception.new("Invalid state #{@state}") end end - protected def done - case @state - when .started? - # https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-cancelioex - # > The application must not free or reuse the OVERLAPPED structure - # associated with the canceled I/O operations until they have completed - if LibC.CancelIoEx(@handle, self) != 0 - @state = :cancelled - @@canceled.push(self) # to increase lifetime - end - end - end - def done! @fiber.cancel_timeout @state = :done @@ -214,7 +188,6 @@ module Crystal::IOCP raise IO::Error.from_os_error(method, error, target: target) end else - operation.done! return value end @@ -244,7 +217,6 @@ module Crystal::IOCP raise IO::Error.from_os_error(method, error, target: target) end else - operation.done! return value end diff --git a/src/crystal/system/win32/socket.cr b/src/crystal/system/win32/socket.cr index 6be2bf04399b..9bf1fd6ac853 100644 --- a/src/crystal/system/win32/socket.cr +++ b/src/crystal/system/win32/socket.cr @@ -142,7 +142,6 @@ module Crystal::System::Socket return ::Socket::Error.from_os_error("ConnectEx", error) end else - operation.done! return nil end @@ -204,7 +203,6 @@ module Crystal::System::Socket return false end else - operation.done! return true end From fafe12ccb386c383800fd71cf0bdb611d727a4f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Tue, 25 Jun 2024 16:59:41 +0200 Subject: [PATCH 5/8] Remove unnecessary state comparison --- src/crystal/system/win32/iocp.cr | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index c7a4212ca0b1..fba6bc128070 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -122,13 +122,8 @@ module Crystal::IOCP end protected def schedule(&) - case @state - when .started? - done! - yield @fiber - else - raise Exception.new("Invalid state #{@state}") - end + done! + yield @fiber end def done! From 04981ba385d691a1a4cc28296b885a3be9b8ad38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Wed, 26 Jun 2024 11:07:44 +0200 Subject: [PATCH 6/8] Allocate `IOCP::OverlappedOperation` on the stack --- src/crystal/system/win32/iocp.cr | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index fba6bc128070..db92c2ef7d92 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -79,7 +79,8 @@ module Crystal::IOCP end def self.run(handle, &) - operation = OverlappedOperation.new(handle) + operation_storage = uninitialized ReferenceStorage(OverlappedOperation) + operation = OverlappedOperation.unsafe_construct(pointerof(operation_storage), handle) yield operation end From 5d545765d6685b1af4715a555624140ea234e952 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Thu, 4 Jul 2024 21:25:55 +0200 Subject: [PATCH 7/8] `try_cancel` --- src/crystal/system/win32/iocp.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index db92c2ef7d92..9b38694df29f 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -132,7 +132,7 @@ module Crystal::IOCP @state = :done end - def cancel! : Bool + def try_cancel : Bool # Microsoft documentation: # The application must not free or reuse the OVERLAPPED structure # associated with the canceled I/O operations until they have completed @@ -159,7 +159,7 @@ module Crystal::IOCP end unless @state.done? - if cancel! + if try_cancel Fiber.suspend end end From 3e451f7802c7e990892a2cab80cc30400afd003d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Thu, 4 Jul 2024 21:28:44 +0200 Subject: [PATCH 8/8] Add comment --- src/crystal/system/win32/iocp.cr | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/crystal/system/win32/iocp.cr b/src/crystal/system/win32/iocp.cr index 9b38694df29f..add5a29c2814 100644 --- a/src/crystal/system/win32/iocp.cr +++ b/src/crystal/system/win32/iocp.cr @@ -160,6 +160,8 @@ module Crystal::IOCP unless @state.done? if try_cancel + # Wait for cancellation to complete. We must not free the operation + # until it's completed. Fiber.suspend end end