IOCP support (draft) #359

PyXiion · 2025-07-09T16:58:50Z

I'm currently working on adding Windows support to the networking part of the library.

I've already done tcp::server, tcp::client and udp::peer classes and ran tests on them, added cross-platform methods for them; write/read instead of recv/send.

// Unix-only, remains available.
co_await client.poll(poll_op::read); 
client.recv(buffer);

// Windows & unix
co_await client.read(buffer);

I need some feedback to improve the PR, I'm not sure about code consistency.

Right now it might not work on Linux systems, because I didn't test in on it, but I'll do, since Linux is my main OS.

Also this is my first contribution, so I'm not sure about commit names (it's kinda messy) and descriptions and other things I probably should know.

# Conflicts: # src/net/socket.cpp

winsock_handle RAII wrapper for WinSock startup/cleanup Refactored socket and I/O logic to use completion ports Crossplatform asynchronous accept_client, connect, read, and write Updated the TCP echo server example to use the new IOCP API Added Windows-specific CMake definitions and compiler flags Temporarily removed UDP support during refactor

- Add write_to/read_from coro methods with timeout support for both platforms: * Unix: using poll() + sendto/recvfrom * Windows: using IOCP overlapped IO with WSASendTo/WSARecvFrom - Add Windows-specific socket initialization with IOCP binding - Maintain existing sendto/recvfrom for Unix compatibility - Update socket handle type to use invalid_handle constant - Fix formatting - address conversion utilities

…uedCompletionStatus for batched events

jbaldwin · 2025-07-10T03:06:31Z

Thanks for opening a PR, this will be an awesome addition! I will take a look and give some feedback, but it might take me a week. I will get to it so please give me some time, thanks!

…d of thread-alert callbacks

…k read/write

tglane

Very cool that we get more and more platforms supported here. I really like you're approach here.

tglane · 2025-07-14T18:12:06Z

include/coro/concepts/executor.hpp

+#elif defined(CORO_PLATFORM_WINDOWS)
+template<typename executor_type>
+concept io_executor = executor<executor_type> and requires(executor_type e, coro::detail::poll_info pi, std::chrono::milliseconds timeout)
+{
+    { e.poll(pi, timeout) } -> std::same_as<coro::task<poll_status>>;
+};
+#endif


I dont't think that it is a good idea to have platform-dependent definitions of concepts. The main point of concepts is to abstract away those things and to have one unified interface.
With that being said, I would also strongly suggest to keep the interface of the ececutor_type::poll function the same over all platforms. If necessary we should add some new type that allows use to have the same definition for that function (e.g. use coro::poll_info instead of the file descriptor and operation on unix systems too as it should contain all the necessary information). The internals of type can differ throughout the platforms but the interfaces should be the same especially for public facing types as I did it with the introduction of kqueue backend. What do you think of that?

Nice idea, it'll be much simpler

tglane · 2025-07-14T18:12:56Z

include/coro/detail/io_notifier_epoll.hpp


    auto watch(fd_t fd, coro::poll_op op, void* data, bool keep = false) -> bool;

+    auto watch(const signal& signal, void* data) -> bool;


This also needs to be added to io_notifier_kqueue.

tglane · 2025-07-14T18:19:00Z

include/coro/io_scheduler.hpp

    }
+    #endif
+#elif defined(CORO_PLATFORM_WINDOWS) && defined(LIBCORO_FEATURE_NETWORKING)
+    auto poll(detail::poll_info& pi, std::chrono::milliseconds timeout) -> coro::task<poll_status>;


See comment on the coro::executor concept (Should be the same interface on all platforms).

tglane · 2025-07-14T18:20:00Z

include/coro/net/dns/resolver.hpp

+#if defined(CORO_PLATFORM_UNIX)
 #include <arpa/inet.h>
 #include <netdb.h>
+#elif defined(CORO_PLATFORM_WINDOWS)


The #elif block is empty so probably unnecessary.

tglane · 2025-07-14T18:24:41Z

include/coro/detail/timer_handle.hpp

 {
-    coro::fd_t  m_fd;
-    const void* m_timer_handle_ptr = nullptr;
+    using native_handle_t = coro::fd_t;


Instead of this we should probably just rename coro::fd_t to coro::native_handle_t and the corresponding file fd.hpp into native_handle.hpp?

➕ I think this makes sense too

tglane · 2025-07-14T19:44:50Z

include/coro/net/read_status.hpp

+
+namespace coro::net
+{
+enum class read_status


What is the reason for not using coro::recv_status? From what I see, they are basically the same, but recv_status gives more detailed error status.

In recv_status/send_status, the values are assigned from <errno.h>. On Windows, you'll have to include the WinAPI headers, which is very undesirable.

You can use a factory (something like make_status)

Hmm yeah I'm wondering how we can unify this, fracturing the two different status' like this seem undesirable from an API usability standpoint

Totally agree with you @jbaldwin.

We could include the windows headers only on windows (guarded by the pre-processor) and than define the enum values according to the correct values depending on the platform used?

tglane · 2025-07-14T19:45:14Z

include/coro/net/write_status.hpp

+
+namespace coro::net
+{
+enum class write_status


What is the reason for not using coro::send_status? From what I see, they are basically the same, but send_status gives more detailed error status.

tglane · 2025-07-14T19:51:56Z

src/detail/signal_win32.cpp

+        m_iocp, 
+        0, 
+        static_cast<int>(io_notifier::completion_key::signal_set),
+        (LPOVERLAPPED)m_data


Avoid c-style casts.

tglane · 2025-07-14T19:52:09Z

src/detail/signal_win32.cpp

+        m_iocp, 
+        0, 
+        static_cast<int>(io_notifier::completion_key::signal_unset),
+        (LPOVERLAPPED)m_data


Avoid c-style casts.

tglane · 2025-07-14T20:15:37Z

src/io_scheduler.cpp

+    m_size.fetch_sub(1, std::memory_order::release);
+    co_return result;
+}
+auto io_scheduler::bind_socket(const net::socket& sock) -> void


I'm not experienced with IOCP and windows in general but I don't like that we have one extra step with bind_socket on windows to use sockets. That is especially bad because it's part of the public facing api of this library. Could you think of some way to get rid of this extra step and embed the body of this function into some already existing place?

Binding a socket in IOCP is mandatory. Accessing the pointer to the IOCP is required to do this. The only way, I think, is to create a getter to io_notifier

I wonder if this would be better logically on the net::socket class and take an io_executor templated parameter?

I'm not familiar with why CreateIoCompletionPort requires the number of threads but we could expose a io_scheduler::thread_count() -> std::size_t like function that it could access to get that information.

Getting m_io_notifier.icop() handle might be more difficult though, its not exactly a platform independent item either.

We could possibly hide this behind a make_socket() -> net::socket for linux/bsd and make_socket(io_executor&) -> net::socket for windows, this might be on case where a slightly different API is required per platform.

Thoughts?

I'm not familiar with why CreateIoCompletionPort requires the number of threads but we could expose a io_scheduler::thread_count() -> std::size_t like function that it could access to get that information.

I clarified a bit in the documentation, you can just pass 0

The maximum number of threads that the operating system can allow to concurrently process I/O completion packets for the I/O completion port. If this parameter is zero, the system allows as many concurrently running threads as there are processors in the system.

Also from WinAPI docs:

NumberOfConcurrentThreads [in]
The maximum number of threads that the operating system can allow to concurrently process I/O completion packets for the I/O completion port. This parameter is ignored if the ExistingCompletionPort parameter is not NULL.

So there is no need for thread count in make_socket, only IOCP pointer

We could possibly hide this behind a make_socket() -> net::socket for linux/bsd and make_socket(io_executor&) -> net::socket for windows, this might be on case where a slightly different API is required per platform.

Also a little about templates, I want to avoid including Windows.h in header files and it's not possible while using templates. We'll have to write something like our own wrapper over WinAPI and WinSock. I think doing it in winsock_handle is a good idea

Example:

auto make_socket(io_executor auto& executor, const socket::options& opts) -> socket { #if defined(CORO_PLATFORM_UNIX) ... #elif defined(CORO_PLATFORM_WINDOWS) auto winsock = detail::initialise_winsock(); socket s{ winsock->new_socket(opts.domain, opts.type) // ::WSASocketW(domain_to_os(opts.domain), socket::type_to_os(opts.type), 0, nullptr, 0, WSA_FLAG_OVERLAPPED) }; if (!s.is_valid()) { throw std::runtime_error{"Failed to create socket."}; } #endif if (opts.blocking == socket::blocking_t::no) { if (s.blocking(socket::blocking_t::no) == false) { throw std::runtime_error{"Failed to set socket to non-blocking mode."}; } } return s; }

I wonder if this would be better logically on the net::socket class and take an io_executor templated parameter?

I really like this idea! Logically, bind_socket should not reside in the io_scheduler and to put it into the socket itself seems like a good workaround.

PyXiion · 2025-07-14T20:45:14Z

Thank you for feedback, @tglane! I'll take care of your comments when I have time.

jbaldwin · 2025-07-15T18:03:47Z

include/coro/io_notifier.hpp

+#include "platform.hpp"

-#if defined(__FreeBSD__) || defined(__APPLE__) || defined(__OpenBSD__) || defined(__NetBSD__)
+#if defined(CORO_PLATFORM_BSD)


jbaldwin · 2025-07-15T18:11:37Z

include/coro/net/tcp/client.hpp

+    }
+}
+
+inline auto client::write(std::span<const char> buffer, std::chrono::milliseconds timeout)


I think recv/send and read/write need to be unified to work the same for all platforms, I don't think having a split api for windows and everything else is a good way to expose users to the library. If we need to migrate linux/bsd to work like how read/write are here I think that would be OK

I want to unify read/write, it's impossible to unify recv/send.

Because in Unix and Windows, the order is completely different: on Unix, we first wait for when we can send/recv, and on Windows, we first send/recv, and then wait for the operation to complete.

Maybe we could get rid of the current implementation of recv/send since they are ultimately only really useable together with poll. The new read/write functions combine those functions so the old ones might not be useful anymore at all?

-> So on unix systems, read/write could just be poll followed by the content of recv/send?

jbaldwin · 2025-07-15T18:13:17Z

include/coro/net/tcp/server.hpp

+     * @return A task resolving to an optional TCP client connection. The value will be set if a client was
+     *         successfully accepted, or std::nullopt if the operation timed out or was cancelled.
+     */
+    auto accept_client(std::chrono::milliseconds timeout = std::chrono::milliseconds{0}) -> coro::task<std::optional<coro::net::tcp::client>>;


Can we unify these as possibly:

auto accept(std::chrono::milliseconds timeout = std::chrono::milliseconds{0}) -> coro::task<std::optional<coro::net::tcp::client>>;?

The linux/bsd versions would need to add timeout support into their respective accept functions since they don't support it right now, but with a unified api I think that should be re-usable

jbaldwin · 2025-07-15T18:35:17Z

include/coro/net/udp/peer.hpp

-            return {static_cast<send_status>(errno), std::span<const char>{}};
-        }
-    }
+    auto sendto(const info& peer_info, const buffer_type& buffer) -> std::pair<send_status, std::span<const char>>;


I think the same question here, how can we unify these functions across all platforms?

jbaldwin · 2025-07-15T18:52:45Z

@PyXiion I had a chance to give this a once over today, this is very very awesome! Thank you for creating this PR.

I think the major item that I'd like to see addressed is how to unify the API between the three platforms, I realize windows is a bit different than linux/bsd but I think we can come up with something that should align the API of this project to be as close as possible for all the platforms as we possibly can.

PyXiion · 2025-07-15T19:13:56Z

@jbaldwin I'm thinking about it too.

I suggest using read/write (read_from/write_to for UDP) as a cross-platform option. On Unix, it uses the poll + recv/send functions, while on Windows, it simply works.

jbaldwin · 2025-07-15T23:17:35Z

Yeah I'd be good with removing send/recv in favor of read/write if it works on all platforms.

PyXiion · 2025-07-16T00:14:57Z

I'm currently working on removing the recv/send functions and am stuck on the statuses. I'm not sure what the point is in separating them into recv and send statuses (or read and write statuses). It would be simpler to use something like the io_status instead of them.

Like asio does https://think-async.com/Asio/asio-1.30.2/doc/asio/reference/error__basic_errors.html

PyXiion added 25 commits June 13, 2025 18:10

Introduce signal abstractions for Unix-based event handling.

83e31ac

IOCP, signals, sockets

cd877c6

Merge branch 'jbaldwin:main' into main

0ff8c84

Platform macros

28c4882

Merge branch 'main' of https://github.com/PyXiion/libcoro

963bfce

Adds read/write methods and noexcept to move constructor.

faa03fa

Uses trailing return type for pipe accessors

0cc7d0c

socket code refactor

f6589e0

Fixes socket API calls and native_handle_t type.

d8de051

Uncomments network-related source files in CMakeLists.

60f2c90

Merge remote-tracking branch 'origin/main'

cacef81

# Conflicts: # src/net/socket.cpp

Uncomments network-related source files in CMakeLists.

627cc66

read_status, write_status

ddd6f1a

Merge remote-tracking branch 'origin/main'

541bf89

Client accept

d6ff687

IOCP write/read, enum for completion keys, IOCP timers

e162793

server::accept_client: fix timeout handling

50a789b

Fix sync requests with sockets

f2d94f4

udp::peer: write_to, read_from cross-platform methods

46116b9

Fixes Linux tests, compilation, and UDP functionality.

8639ed5

Add notes and clarify code behavior in IOCP implementation details.

a009c98

Remove unused file

bbb274f

io_notifier_iocp: using GetQueuedCompletionStatusEx instead of GetQue…

6a2f19d

…uedCompletionStatus for batched events

PyXiion added 3 commits July 13, 2025 22:30

timer_handle: remove mutables, use RegisterWaitForSingleObject instea…

7dc9562

…d of thread-alert callbacks

handle close on zero bytes in the client, generic function for WinSoc…

9c7a26f

…k read/write

fix some Codacy problems, remove mutable from signal_win32

86a78b5

tglane suggested changes Jul 14, 2025

View reviewed changes

jbaldwin reviewed Jul 15, 2025

View reviewed changes

jbaldwin mentioned this pull request Sep 16, 2025

Road to 1.0.0 #395

Open


		auto watch(fd_t fd, coro::poll_op op, void* data, bool keep = false) -> bool;

		auto watch(const signal& signal, void* data) -> bool;

IOCP support (draft) #359

Are you sure you want to change the base?

IOCP support (draft) #359

Uh oh!

Conversation

PyXiion commented Jul 9, 2025

Uh oh!

jbaldwin commented Jul 10, 2025

Uh oh!

tglane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Example:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PyXiion commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbaldwin Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbaldwin commented Jul 15, 2025

Uh oh!

PyXiion commented Jul 15, 2025

Uh oh!

jbaldwin commented Jul 15, 2025

Uh oh!

PyXiion commented Jul 16, 2025

Uh oh!

Uh oh!

jbaldwin Jul 15, 2025 •

edited

Loading