[PATCH 0/1] MR8454: ntdll: Don't skip synchronous read when serial read timeout is infinite.
If get_io_timeouts sets the interval timeout to 0, it means that the read should return immediately even if there is no data. If get_io_timeouts sets the interval timeout to -1, it means that the read operation should wait indefinitely for the next byte. In either case, the interval timer is not used, so there's no need to worry about forgetting to start it, and the delay in returning data was causing a problem in Ti99Hdx. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=42882 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454
This seems fine, but I'm concerned, because I think we probably want to replace this code flow with something more like what sockets have, in order to make sure that requests are handled in a FIFO way. What problem was the delay causing? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454#note_108391
On Mon Jun 30 19:34:13 2025 +0000, Elizabeth Figura wrote:
This seems fine, but I'm concerned, because I think we probably want to replace this code flow with something more like what sockets have, in order to make sure that requests are handled in a FIFO way. What problem was the delay causing? For some reason, skipping the synchronous read causes the app to not receive any bytes. Although the app is meant to connect to an ancient TI-99 computer, you can see the problem by simply connecting the app's serial port to another serial port with a null modem.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454#note_108393
From: Alex Henrie <alexhenrie24(a)gmail.com> If get_io_timeouts sets the interval timeout to 0, it means that the read should return immediately even if there is no data. If get_io_timeouts sets the interval timeout to -1, it means that the read operation should wait indefinitely for the next byte. In either case, the interval timer is not used, so there's no need to worry about forgetting to start it, and the delay in returning data was causing a problem in Ti99Hdx. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=42882 --- dlls/ntdll/unix/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 3d590e4915b..5db6f110da0 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -5760,7 +5760,7 @@ NTSTATUS WINAPI NtReadFile( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, vo skip the synchronous read to make sure that the server starts the read interval timer after the first read */ if ((status = get_io_timeouts( handle, type, length, TRUE, &timeouts ))) goto err; - if (timeouts.interval) + if (timeouts.interval > 0) { status = register_async_file_read( handle, event, apc, apc_user, iosb_ptr, buffer, total, length, FALSE ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8454
For some reason, skipping the synchronous read causes the app to not receive any bytes.
What reason is that? I think we should understand this first, or we're potentially hiding another bug. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454#note_108405
On Mon Jun 30 21:16:39 2025 +0000, Elizabeth Figura wrote:
For some reason, skipping the synchronous read causes the app to not receive any bytes. What reason is that? I think we should understand this first, or we're potentially hiding another bug. I can try to dig deeper when I get some free time, though in my opinion there's no harm in reverting to the pre-regression behavior in this case. When I originally wrote this code, it certainly wasn't my intention to skip the synchronous read when the timeout is infinite. That was an oversight on my part.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454#note_108406
On Mon Jun 30 21:26:07 2025 +0000, Alex Henrie wrote:
I can try to dig deeper when I get some free time, though in my opinion there's no harm in reverting to the pre-regression behavior in this case. When I originally wrote this code, it certainly wasn't my intention to skip the synchronous read when the timeout is infinite. That was an oversight on my part. It helps a lot that [Ti99Hdx](https://hexbus.com/ti99geek/Projects/ti99hdx/ti99hdx_server.html) is [open source](https://hexbus.com/ti99geek/Projects/ti99hdx/Files/ti99hdx113a_src.zip). A timer set up via SetTimer calls the ReadSerialData function in Ti99HdxCom.cpp once every 10 milliseconds, and ReadSerialData does the following:
```cpp if (!ClearCommError(hSerCom, &dwErrors, &comStat)) { return 0; } if ((dwRead = comStat.cbInQue) > 0) { if (dwRead > (unsigned int) Maxsize) dwRead = Maxsize; if (!ReadFile(hSerCom, Buffer, dwRead, &dwRead, &osSerCom)) { } else { return (int) dwRead; } } return 0; ``` The problem is that the program never actually checks the result of the osSerCom OVERLAPPED struct. It's requesting an asynchronous read, but then it expects data to be returned synchronously. In fact, it only calls ReadFile after checking that there is already data in the queue to be read. If no data is ever returned synchronously, dwRead is always 0, and the program doesn't do anything. Ti99Hdx never sets the serial interval timeout; it uses the default infinite timeout. Should Wine return some data synchronously even if the program did set an interval timeout? Probably. On the other hand, is there actually a program that needs that? It's weird enough that we found a program that requests asynchronous I/O that it doesn't really want. Why would a program go so far as to set a timeout if it's not actually going to check the result of the asynchronous operation? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454#note_108668
On Wed Jul 2 18:35:06 2025 +0000, Alex Henrie wrote:
It helps a lot that [Ti99Hdx](https://hexbus.com/ti99geek/Projects/ti99hdx/ti99hdx_server.html) is [open source](https://hexbus.com/ti99geek/Projects/ti99hdx/Files/ti99hdx113a_src.zip). A timer set up via SetTimer calls the ReadSerialData function in Ti99HdxCom.cpp once every 10 milliseconds, and ReadSerialData does the following: ```cpp if (!ClearCommError(hSerCom, &dwErrors, &comStat)) { return 0; } if ((dwRead = comStat.cbInQue) > 0) { if (dwRead > (unsigned int) Maxsize) dwRead = Maxsize; if (!ReadFile(hSerCom, Buffer, dwRead, &dwRead, &osSerCom)) { } else { return (int) dwRead; } } return 0; ``` The problem is that the program never actually checks the result of the osSerCom OVERLAPPED struct. It's requesting an asynchronous read, but then it expects data to be returned synchronously. In fact, it only calls ReadFile after checking that there is already data in the queue to be read. If no data is ever returned synchronously, dwRead is always 0, and the program doesn't do anything. Ti99Hdx never sets the serial interval timeout; it uses the default infinite timeout. Should Wine return some data synchronously even if the program did set an interval timeout? Probably. On the other hand, is there actually a program that needs that? It's weird enough that we found a program that requests asynchronous I/O that it doesn't really want. Why would a program go so far as to set a timeout if it's not actually going to check the result of the asynchronous operation? That makes sense. It's a bit arcane, but I'm not sure I'd call it buggy. In any case set_async_direct_result is explicitly designed to allow for this. I'll probably submit a patch for that later, but I think this patch is fine for now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454#note_108670
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8454
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Elizabeth Figura (＠zfigura)