Open
Conversation
Move initialize_threading() to run before pool_mutex is accessed in lwt_unix_start_job, rather than only inside the DETACH/SWITCH case. On Windows, CRITICAL_SECTION must be explicitly initialized via InitializeCriticalSection before use. Unlike pthreads where a zero-initialized mutex (PTHREAD_MUTEX_INITIALIZER) is valid, a zero-initialized CRITICAL_SECTION is invalid. The previous code could access pool_mutex before initialize_threading() was called, causing crashes or undefined behavior on Windows when the first job used async_method != NONE. initialize_threading() is idempotent (guarded by threading_initialized flag), so calling it earlier is safe and has no effect on Unix platforms.
Switch from STARTUPINFO to STARTUPINFOEX with PROC_THREAD_ATTRIBUTE_HANDLE_LIST to explicitly specify which handles the child process should inherit. Previously, using SetHandleInformation + bInheritHandles=TRUE caused a race condition where all inheritable handles (including those from concurrent process spawns) would leak into child processes. The new approach uses the Microsoft-recommended PROC_THREAD_ATTRIBUTE_HANDLE_LIST to restrict inheritance to only the intended stdin/stdout/stderr handles. The handle list is deduplicated since UpdateProcThreadAttribute requires unique handle values (e.g. when stdout and stderr point to the same handle). Also fixes the comment to correctly reference fd0, fd1, fd2 instead of fd1, fd2, fd3.
Add a new Windows-specific waitpid job (windows_waitpid_job.c) that uses OpenProcess + WaitForSingleObject + GetExitCodeProcess instead of the POSIX-style Unix.waitpid which doesn't work properly on Windows. The new implementation: - Runs in a worker thread via the Lwt job system for proper async behavior - Supports WNOHANG via a 0-timeout WaitForSingleObject call - Returns (0, WEXITED 0) for WNOHANG when the process is still running, matching the POSIX convention - Returns (pid, WEXITED exit_code) when the process has exited The OCaml side dispatches to _win32_waitpid on Windows (which runs the new C job) instead of _waitpid (which called Unix.waitpid synchronously).
On Windows, select() only works with socket handles, not with pipe handles or regular file handles. Calling wait_read or wait_write on a blocking pipe fd would fail because the underlying select() call cannot handle non-socket HANDLEs. Skip the wait_read/wait_write calls on Windows (guarded by Sys.win32) and let the worker thread handle blocking directly. This is safe because Sys.win32 is a compile-time constant, so there is no runtime overhead on Unix platforms. Affected call sites: read, pread, read_bigarray, write, pwrite, write_bigarray (6 total).
Collaborator
raphael-proust
left a comment
raphael-proust
left a comment
There was a problem hiding this comment.
thanks a lot for the contribution that's outside of my area, it's very valuable <3
i'll try to get someone to read the windows-specific C code before merging
src/unix/lwt_unix.cppo.ml
Outdated
| wait_read ch >>= fun () -> | ||
| (* On Windows, select() doesn't work with pipe handles, so skip | ||
| wait_read and let the worker thread handle blocking directly. *) | ||
| (if Sys.win32 then Lwt.return_unit else wait_read ch) >>= fun () -> |
Collaborator
There was a problem hiding this comment.
somewhat nitpicky but also i want to keep the code very readable to ease maintenance…
I think i'd rather have the branch hoisted up so that the AST is flatter (if wider)
Lazy.force ch.blocking >>= function
| true when Sys.win32 ->
(* comment *)
windows code
| true ->
linux code true
| false ->
linux code false
Author
There was a problem hiding this comment.
I've refactored this usage pattern.
| } | ||
| } | ||
|
|
||
| #endif |
Collaborator
There was a problem hiding this comment.
I don't have the bandwidth to review this rn. I'm completely unfamiliar with the specifics of windows. I'll try to ping someone who can help with that part.
| if (async_method != LWT_UNIX_ASYNC_METHOD_NONE) { | ||
| initialize_threading(); | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have been working on ocluster/obuilder on Windows and have run into several difficulties with lwt. I would like to contribute these fixes back to the codebase. I have divided it into four separate commits to make it easier to review. Each commit has a detailed description.
I note that some of the code generation was done using an AI agent. However, I have reviewed every line of these commits and tested them on both Windows 2025 and Windows 2019.