[PATCH 0/1] MR8454: ntdll: Don't skip synchronous read when serial read timeout is infinite.

[Alex Henrie (＠alexhenrie)]

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

[Elizabeth Figura (＠zfigura)]

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?

[Alex Henrie (＠alexhenrie)]

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.

[PATCH 1/1] ntdll: Don't skip synchronous read when serial read timeout is infinite.

[Alex Henrie]

From: Alex Henrie

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 );

[Elizabeth Figura (＠zfigura)]

> 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.

[Alex Henrie (＠alexhenrie)]

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.

[Alex Henrie (＠alexhenrie)]

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?

[Elizabeth Figura (＠zfigura)]

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:
> 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.

[PATCH 0/1] MR8454: ntdll: Don't skip synchronous read when serial read timeout is infinite. - approved

[Elizabeth Figura (＠zfigura)]

This merge request was approved by Elizabeth Figura.