[PATCH] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Takashi Yano
- Though this rarely happens, sometimes the first printing of non-
  cygwin process does not displayed correctly. To fix this issue,
  the code for waiting for forwarding by master_fwd_thread is revised.
---
 winsup/cygwin/fhandler.h      |  1 +
 winsup/cygwin/fhandler_tty.cc | 16 +++++++++++++---
 winsup/cygwin/tty.cc          |  1 +
 winsup/cygwin/tty.h           |  1 +
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c0d56b4da..f3301b059 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2201,6 +2201,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   void setup_locale (void);
   void set_freeconsole_on_close (bool val);
+  void wait_forwarding (void);
 };
 
 #define __ptsname(buf, unit) __small_sprintf ((buf), "/dev/pty%d", (unit))
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index a5db0967b..c839b9806 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1109,7 +1109,7 @@ skip_console_setting:
     }
   else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
     {
-      Sleep (20);
+      wait_forwarding ();
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -1151,7 +1151,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
       SetConsoleMode (get_handle (), mode & ~ENABLE_ECHO_INPUT);
     }
   if (get_ttyp ()->switch_to_pcon_out)
-    Sleep (20); /* Wait for pty_master_fwd_thread() */
+    wait_forwarding (); /* Wait for pty_master_fwd_thread() */
   get_ttyp ()->pcon_pid = 0;
   get_ttyp ()->switch_to_pcon_in = false;
   get_ttyp ()->switch_to_pcon_out = false;
@@ -2718,7 +2718,7 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
       DWORD mode = ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT;
       SetConsoleMode (get_output_handle (), mode);
       if (!get_ttyp ()->switch_to_pcon_out)
- Sleep (20);
+ wait_forwarding ();
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -2991,6 +2991,15 @@ pty_master_thread (VOID *arg)
   return ((fhandler_pty_master *) arg)->pty_master_thread ();
 }
 
+void
+fhandler_pty_slave::wait_forwarding (void)
+{
+  const DWORD time_to_wait = 40;
+  DWORD elasped = GetTickCount () - get_ttyp ()->last_fwd_time;
+  if (elasped < time_to_wait)
+    Sleep (time_to_wait - elasped);
+}
+
 DWORD
 fhandler_pty_master::pty_master_fwd_thread ()
 {
@@ -3000,6 +3009,7 @@ fhandler_pty_master::pty_master_fwd_thread ()
   termios_printf ("Started.");
   for (;;)
     {
+      get_ttyp ()->last_fwd_time = GetTickCount ();
       if (!ReadFile (from_slave, outbuf, sizeof outbuf, &rlen, NULL))
  {
   termios_printf ("ReadFile for forwarding failed, %E");
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index 695ce91f1..095838f1b 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -245,6 +245,7 @@ tty::init ()
   num_pcon_attached_slaves = 0;
   term_code_page = 0;
   need_redraw_screen = false;
+  last_fwd_time = GetTickCount ();
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index cd4c0ed44..d6b22359a 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -106,6 +106,7 @@ private:
   int num_pcon_attached_slaves;
   UINT term_code_page;
   bool need_redraw_screen;
+  DWORD last_fwd_time;
 
 public:
   HANDLE from_master () const { return _from_master; }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Corinna Vinschen-2
Hi Takashi,

On Jan 20 11:50, Takashi Yano wrote:

> - Though this rarely happens, sometimes the first printing of non-
>   cygwin process does not displayed correctly. To fix this issue,
>   the code for waiting for forwarding by master_fwd_thread is revised.
> ---
> [...]
> +void
> +fhandler_pty_slave::wait_forwarding (void)
> +{
> +  const DWORD time_to_wait = 40;
> +  DWORD elasped = GetTickCount () - get_ttyp ()->last_fwd_time;
> +  if (elasped < time_to_wait)
> +    Sleep (time_to_wait - elasped);
> +}
> +
Are these 40 ms an experimental value or is that based on knowledge
of implementation details?  The real question is, isn't there any
other, more reliable indicator to see if forwarding will work?


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Takashi Yano
On Mon, 20 Jan 2020 11:39:39 +0100
Corinna Vinschen wrote:
> Are these 40 ms an experimental value or is that based on knowledge
> of implementation details?

It is experimental value which I measured in several environment.

> The real question is, isn't there any
> other, more reliable indicator to see if forwarding will work?

I have tried another implementation and confirmed it works as expected.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Takashi Yano
- Though this rarely happens, sometimes the first printing of non-
  cygwin process does not displayed correctly. To fix this issue,
  the code for waiting for forwarding by master_fwd_thread is revised.
---
 winsup/cygwin/fhandler_tty.cc | 15 +++++++++++----
 winsup/cygwin/tty.cc          |  1 +
 winsup/cygwin/tty.h           |  1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index a5db0967b..6c050e32a 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -648,7 +648,7 @@ fhandler_pty_master::process_slave_output (char *buf, size_t len, int pktmode_on
       /* If echo pipe has data (something has been typed or pasted), prefer
          it over slave output. */
       if (echo_cnt > 0)
-       {
+ {
   if (!ReadFile (echo_r, outbuf, rlen, &n, NULL))
     {
       termios_printf ("ReadFile on echo pipe failed, %E");
@@ -1109,7 +1109,7 @@ skip_console_setting:
     }
   else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
     {
-      Sleep (20);
+      cygwait (get_ttyp ()->fwd_done, INFINITE);
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -1151,7 +1151,8 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
       SetConsoleMode (get_handle (), mode & ~ENABLE_ECHO_INPUT);
     }
   if (get_ttyp ()->switch_to_pcon_out)
-    Sleep (20); /* Wait for pty_master_fwd_thread() */
+    /* Wait for pty_master_fwd_thread() */
+    cygwait (get_ttyp ()->fwd_done, INFINITE);
   get_ttyp ()->pcon_pid = 0;
   get_ttyp ()->switch_to_pcon_in = false;
   get_ttyp ()->switch_to_pcon_out = false;
@@ -2202,6 +2203,8 @@ fhandler_pty_master::close ()
     }
   release_output_mutex ();
   master_fwd_thread->terminate_thread ();
+  CloseHandle (get_ttyp ()->fwd_done);
+  get_ttyp ()->fwd_done = NULL;
  }
     }
 
@@ -2718,7 +2721,7 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
       DWORD mode = ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT;
       SetConsoleMode (get_output_handle (), mode);
       if (!get_ttyp ()->switch_to_pcon_out)
- Sleep (20);
+ cygwait (get_ttyp ()->fwd_done, INFINITE);
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -3000,11 +3003,14 @@ fhandler_pty_master::pty_master_fwd_thread ()
   termios_printf ("Started.");
   for (;;)
     {
+      if (::bytes_available (rlen, from_slave) && rlen == 0)
+ SetEvent (get_ttyp ()->fwd_done);
       if (!ReadFile (from_slave, outbuf, sizeof outbuf, &rlen, NULL))
  {
   termios_printf ("ReadFile for forwarding failed, %E");
   break;
  }
+      ResetEvent (get_ttyp ()->fwd_done);
       ssize_t wlen = rlen;
       char *ptr = outbuf;
       if (get_pseudo_console ())
@@ -3367,6 +3373,7 @@ fhandler_pty_master::setup ()
       errstr = "pty master forwarding thread";
       goto err;
     }
+  get_ttyp ()->fwd_done = CreateEvent (&sec_none, true, false, NULL);
 
   setup_pseudoconsole ();
 
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index 695ce91f1..ef9bbc1ff 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -245,6 +245,7 @@ tty::init ()
   num_pcon_attached_slaves = 0;
   term_code_page = 0;
   need_redraw_screen = false;
+  fwd_done = NULL;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index cd4c0ed44..b291fd3c1 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -106,6 +106,7 @@ private:
   int num_pcon_attached_slaves;
   UINT term_code_page;
   bool need_redraw_screen;
+  HANDLE fwd_done;
 
 public:
   HANDLE from_master () const { return _from_master; }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Corinna Vinschen-2
On Jan 21 11:22, Takashi Yano wrote:
> - Though this rarely happens, sometimes the first printing of non-
>   cygwin process does not displayed correctly. To fix this issue,
>   the code for waiting for forwarding by master_fwd_thread is revised.

Looks good.  Pushed.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Takashi Yano
Hi Corinna,

On Tue, 21 Jan 2020 10:37:35 +0100
Corinna Vinschen wrote:
> On Jan 21 11:22, Takashi Yano wrote:
> > - Though this rarely happens, sometimes the first printing of non-
> >   cygwin process does not displayed correctly. To fix this issue,
> >   the code for waiting for forwarding by master_fwd_thread is revised.
>
> Looks good.  Pushed.

First of all, I have to apologize for insufficient test.
With this patch, the following test case frequently displays
its output incorrectly.

Sometimes it is just
AAAA
or
AAAA
AAAA (duplicated)
or
AAAA
<blank line>

Of course, the expected ersult is:
AAAA
BBBB

/* Test code */
#include <windows.h>
#include <stdio.h>

int main()
{
    DWORD n;
    printf("AAAA\n");
    WriteConsole(GetStdHandle(STD_OUTPUT_HANDLE), "BBBB\r\n", 6, &n, 0);
    return 0;
}

I looked into this problem and found that the cause. What we
really need is not waiting for forwarding by pty_master_fwd_thread
but is waiting for forwarding by pseudo console itself. In other
words, the latency between slave write to get_output_handle()
and master read from from_slave pipe is dominant.

We cannot touch the forwarding process in pseudo console, therefore
the strategy like this patch can not be used. So, we might have to
revert to dumb Sleep() wait.

As for the wait time, I have measured the latency of pseudo console
in following machines.

Machine1: Ryzen 9 3950X (3.5GHz)
Machine2: Core i7 6700K (4.0GHz)
Machine3: Xeon X5670 (2.93GHz)
Machine4: Core i7 870 (2.93GHz)
Machine5: PentinumD 930 (3.0GHz)  <- Very old!

Measuring is done 1000 times for each machine. The results are as
follows.

Machine1:
Count  Latency[ms]
  416   0
  222  15
  362  16

Machine2:
Count  Latency[ms]
  421   0
  185  15
  394  16

Machine3:
Count  Latency[ms]
  439   0
  199  15
  362  16

Machine4:
Count  Latency[ms]
  202  15
  340  16
  343  31
  115  32

Machine5:
Count  Latency[ms]
  140 15
  284 16
  410 31
  159 32
    7 47

Obviously, the latency depends on the machine performance.
Therefore, I will propose new patch which uses dumb Sleep()
with on-the-fly performance test.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding by master_fwd_thread.

Takashi Yano
On Thu, 23 Jan 2020 01:00:11 +0900
Takashi Yano wrote:

> /* Test code */
> #include <windows.h>
> #include <stdio.h>
>
> int main()
> {
>     DWORD n;
>     printf("AAAA\n");
>     WriteConsole(GetStdHandle(STD_OUTPUT_HANDLE), "BBBB\r\n", 6, &n, 0);
>     return 0;
> }

The problem occurs when the code above is compiled as cygwin binary.
Not windows native binary. In other words, use cygwin-gcc rather than
mingw-gcc.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
In reply to this post by Takashi Yano
- After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
  cygwin programs which call both printf() and WriteConsole() are
  frequently distorted. This patch reverts waiting function to dumb
  Sleep().
---
 winsup/cygwin/fhandler_tty.cc | 30 +++++++++++++++++++-----------
 winsup/cygwin/tty.cc          |  3 ++-
 winsup/cygwin/tty.h           |  3 ++-
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 1710f2520..404216bf6 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1109,7 +1109,9 @@ skip_console_setting:
     }
   else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
     {
-      cygwait (get_ttyp ()->fwd_done, INFINITE);
+      DWORD elasped = GetTickCount () - get_ttyp ()->last_push_time;
+      if (elasped < get_ttyp ()->max_fwd_latency)
+ Sleep (get_ttyp ()->max_fwd_latency - elasped);
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -1151,8 +1153,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
       SetConsoleMode (get_handle (), mode & ~ENABLE_ECHO_INPUT);
     }
   if (get_ttyp ()->switch_to_pcon_out)
-    /* Wait for pty_master_fwd_thread() */
-    cygwait (get_ttyp ()->fwd_done, INFINITE);
+    Sleep (get_ttyp ()->max_fwd_latency);
   get_ttyp ()->pcon_pid = 0;
   get_ttyp ()->switch_to_pcon_in = false;
   get_ttyp ()->switch_to_pcon_out = false;
@@ -1187,6 +1188,8 @@ fhandler_pty_slave::push_to_pcon_screenbuffer (const char *ptr, size_t len)
     if (!try_reattach_pcon ())
       goto detach;
 
+  get_ttyp ()->last_push_time = GetTickCount ();
+
   char *buf;
   size_t nlen;
   DWORD origCP;
@@ -2210,8 +2213,6 @@ fhandler_pty_master::close ()
     }
   release_output_mutex ();
   master_fwd_thread->terminate_thread ();
-  CloseHandle (get_ttyp ()->fwd_done);
-  get_ttyp ()->fwd_done = NULL;
  }
     }
 
@@ -2728,7 +2729,11 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
       DWORD mode = ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT;
       SetConsoleMode (get_output_handle (), mode);
       if (!get_ttyp ()->switch_to_pcon_out)
- cygwait (get_ttyp ()->fwd_done, INFINITE);
+ {
+  DWORD elasped = GetTickCount () - get_ttyp ()->last_push_time;
+  if (elasped < get_ttyp ()->max_fwd_latency)
+    Sleep (get_ttyp ()->max_fwd_latency - elasped);
+ }
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -3010,14 +3015,12 @@ fhandler_pty_master::pty_master_fwd_thread ()
   termios_printf ("Started.");
   for (;;)
     {
-      if (::bytes_available (rlen, from_slave) && rlen == 0)
- SetEvent (get_ttyp ()->fwd_done);
       if (!ReadFile (from_slave, outbuf, sizeof outbuf, &rlen, NULL))
  {
   termios_printf ("ReadFile for forwarding failed, %E");
   break;
  }
-      ResetEvent (get_ttyp ()->fwd_done);
+
       ssize_t wlen = rlen;
       char *ptr = outbuf;
       if (get_pseudo_console ())
@@ -3025,7 +3028,13 @@ fhandler_pty_master::pty_master_fwd_thread ()
   /* Avoid duplicating slave output which is already sent to
      to_master_cyg */
   if (!get_ttyp ()->switch_to_pcon_out)
-    continue;
+    {
+      DWORD elasped = GetTickCount () - get_ttyp ()->last_push_time;
+      if (get_ttyp ()->last_push_time &&
+  elasped > get_ttyp ()->max_fwd_latency)
+ get_ttyp ()->max_fwd_latency = elasped;
+      continue;
+    }
 
   /* Avoid setting window title to "cygwin-console-helper.exe" */
   int state = 0;
@@ -3380,7 +3389,6 @@ fhandler_pty_master::setup ()
       errstr = "pty master forwarding thread";
       goto err;
     }
-  get_ttyp ()->fwd_done = CreateEvent (&sec_none, true, false, NULL);
 
   setup_pseudoconsole ();
 
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index ef9bbc1ff..b290a41e1 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -245,7 +245,8 @@ tty::init ()
   num_pcon_attached_slaves = 0;
   term_code_page = 0;
   need_redraw_screen = false;
-  fwd_done = NULL;
+  max_fwd_latency = 20;
+  last_push_time = 0;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index b291fd3c1..ba2b9074a 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -106,7 +106,8 @@ private:
   int num_pcon_attached_slaves;
   UINT term_code_page;
   bool need_redraw_screen;
-  HANDLE fwd_done;
+  DWORD max_fwd_latency;
+  DWORD last_push_time;
 
 public:
   HANDLE from_master () const { return _from_master; }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
- After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
  cygwin programs which call both printf() and WriteConsole() are
  frequently distorted. This patch reverts waiting function to dumb
  Sleep().
---
 winsup/cygwin/fhandler_tty.cc | 30 +++++++++++++++++++-----------
 winsup/cygwin/tty.cc          |  3 ++-
 winsup/cygwin/tty.h           |  3 ++-
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 1710f2520..69ef56069 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1109,7 +1109,9 @@ skip_console_setting:
     }
   else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
     {
-      cygwait (get_ttyp ()->fwd_done, INFINITE);
+      DWORD elasped = GetTickCount () - get_ttyp ()->last_push_time;
+      if (elasped < get_ttyp ()->max_fwd_latency)
+ Sleep (get_ttyp ()->max_fwd_latency - elasped);
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -1151,8 +1153,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
       SetConsoleMode (get_handle (), mode & ~ENABLE_ECHO_INPUT);
     }
   if (get_ttyp ()->switch_to_pcon_out)
-    /* Wait for pty_master_fwd_thread() */
-    cygwait (get_ttyp ()->fwd_done, INFINITE);
+    Sleep (get_ttyp ()->max_fwd_latency);
   get_ttyp ()->pcon_pid = 0;
   get_ttyp ()->switch_to_pcon_in = false;
   get_ttyp ()->switch_to_pcon_out = false;
@@ -1187,6 +1188,8 @@ fhandler_pty_slave::push_to_pcon_screenbuffer (const char *ptr, size_t len)
     if (!try_reattach_pcon ())
       goto detach;
 
+  get_ttyp ()->last_push_time = GetTickCount ();
+
   char *buf;
   size_t nlen;
   DWORD origCP;
@@ -2210,8 +2213,6 @@ fhandler_pty_master::close ()
     }
   release_output_mutex ();
   master_fwd_thread->terminate_thread ();
-  CloseHandle (get_ttyp ()->fwd_done);
-  get_ttyp ()->fwd_done = NULL;
  }
     }
 
@@ -2728,7 +2729,11 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
       DWORD mode = ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT;
       SetConsoleMode (get_output_handle (), mode);
       if (!get_ttyp ()->switch_to_pcon_out)
- cygwait (get_ttyp ()->fwd_done, INFINITE);
+ {
+  DWORD elasped = GetTickCount () - get_ttyp ()->last_push_time;
+  if (elasped < get_ttyp ()->max_fwd_latency)
+    Sleep (get_ttyp ()->max_fwd_latency - elasped);
+ }
       if (get_ttyp ()->pcon_pid == 0 ||
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
@@ -3010,14 +3015,12 @@ fhandler_pty_master::pty_master_fwd_thread ()
   termios_printf ("Started.");
   for (;;)
     {
-      if (::bytes_available (rlen, from_slave) && rlen == 0)
- SetEvent (get_ttyp ()->fwd_done);
       if (!ReadFile (from_slave, outbuf, sizeof outbuf, &rlen, NULL))
  {
   termios_printf ("ReadFile for forwarding failed, %E");
   break;
  }
-      ResetEvent (get_ttyp ()->fwd_done);
+
       ssize_t wlen = rlen;
       char *ptr = outbuf;
       if (get_pseudo_console ())
@@ -3025,7 +3028,13 @@ fhandler_pty_master::pty_master_fwd_thread ()
   /* Avoid duplicating slave output which is already sent to
      to_master_cyg */
   if (!get_ttyp ()->switch_to_pcon_out)
-    continue;
+    {
+      DWORD elasped = GetTickCount () - get_ttyp ()->last_push_time;
+      if (rlen && get_ttyp ()->last_push_time &&
+  elasped > get_ttyp ()->max_fwd_latency)
+ get_ttyp ()->max_fwd_latency = elasped;
+      continue;
+    }
 
   /* Avoid setting window title to "cygwin-console-helper.exe" */
   int state = 0;
@@ -3380,7 +3389,6 @@ fhandler_pty_master::setup ()
       errstr = "pty master forwarding thread";
       goto err;
     }
-  get_ttyp ()->fwd_done = CreateEvent (&sec_none, true, false, NULL);
 
   setup_pseudoconsole ();
 
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index ef9bbc1ff..b290a41e1 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -245,7 +245,8 @@ tty::init ()
   num_pcon_attached_slaves = 0;
   term_code_page = 0;
   need_redraw_screen = false;
-  fwd_done = NULL;
+  max_fwd_latency = 20;
+  last_push_time = 0;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index b291fd3c1..ba2b9074a 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -106,7 +106,8 @@ private:
   int num_pcon_attached_slaves;
   UINT term_code_page;
   bool need_redraw_screen;
-  HANDLE fwd_done;
+  DWORD max_fwd_latency;
+  DWORD last_push_time;
 
 public:
   HANDLE from_master () const { return _from_master; }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Corinna Vinschen-2
On Jan 23 13:30, Takashi Yano wrote:
> - After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
>   cygwin programs which call both printf() and WriteConsole() are
>   frequently distorted. This patch reverts waiting function to dumb
>   Sleep().

I understand the need for this change, but isn't there any other
way to detect if the pseudo console being ready?  E. g., something
in the HPCON_INTERNAL struct or so?


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Koichi Murase
> On Jan 23 13:30, Takashi Yano wrote:
> > - After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
> >   cygwin programs which call both printf() and WriteConsole() are
> >   frequently distorted. This patch reverts waiting function to dumb
> >   Sleep().

    Hi, I have a question related to this patch. (When I have a
question on a specific patch like this, which mailing list should I
come?  If I should not make a reply to the original cygwin-patch
mailing list, let me apologize in advance.  If so, I'll move to
cygwin mailing list.)

    When I try to use the recent commit 6d79e0a58 (tag: newlib-3.3.0),
any Cygwin program fails to start leaving the following message:

      0 [main] XXXX (YYYY) shared_info::initialize: size of shared
      memory region changed from 50104 to 49080

where XXXX and YYYY are the program name and PID.  I also tried with
the current master branch 8f502bd33, and the result was the same.  I
tested each commit one by one, and found that this problem is caused
after this patch:

  6cc299f0e - (2 days ago) Cygwin: pty: Revise code waiting for
  forwarding by master_fwd_thread. - Takashi Yano

In fact, if I drop this commit from the master branch, the problem
disappears.


    But, as there are no related reports here, I suspect this is the
problem specific to my environment.  In particular, I suspect that
this is caused by the compatibility of different versions of
`cygwin1.dll'.  Currently, when I try to use the new `cygwin1.dll', I
just replace `C:\cygwin64\bin\cygwin1.dll' with the version I built
from recent a commit (`new-cygwin1.dll') following the instruction for
snapshots which is found at

  https://cygwin.com/faq.html#faq.setup.snapshots

Here my question is, if this is caused by the way I try the new
version, what is the correct way to try the latest version built from
a commit in the git repository (do I need to rebuild all the
toolchain)?  Or, is this problem caused by other conditions?  I would
appreciate it if you could provide me some hints.


    Here is some trials in command prompt:

  C:\cygwin64\bin>bash
        0 [main] bash (18936) shared_info::initialize: size of shared
  memory region changed from 50104 to 49080

  C:\cygwin64\bin>dash
        0 [main] dash (7900) shared_info::initialize: size of shared
  memory region changed from 50104 to 49080

  C:\cygwin64\bin>stty
        0 [main] stty (2920) shared_info::initialize: size of shared
  memory region changed from 50104 to 49080

  C:\cygwin64\bin>cat
        0 [main] cat (21340) shared_info::initialize: size of shared
  memory region changed from 50104 to 49080

  C:\cygwin64\bin>mintty

mintty fails without any messages.


Thank you,

Koichi
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Koichi Murase
2020年1月23日(木) 21:39 Koichi Murase <[hidden email]>:
> > On Jan 23 13:30, Takashi Yano wrote:
> > > - After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
> > >   cygwin programs which call both printf() and WriteConsole() are
> > >   frequently distorted. This patch reverts waiting function to dumb
> > >   Sleep().

I'm sorry, I made a reply to a wrong mail (with a similar subject).  I
should have made this reply to the original version of the patch.
Sorry for the confusion.

Koichi
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
In reply to this post by Koichi Murase
On Thu, 23 Jan 2020 21:39:50 +0800
Koichi Murase wrote:
>       0 [main] XXXX (YYYY) shared_info::initialize: size of shared
>       memory region changed from 50104 to 49080

Is there any process alived using diffrent version of cygwin1.dll?

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
In reply to this post by Corinna Vinschen-2
On Thu, 23 Jan 2020 13:51:54 +0100
Corinna Vinschen wrote:
> On Jan 23 13:30, Takashi Yano wrote:
> > - After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
> >   cygwin programs which call both printf() and WriteConsole() are
> >   frequently distorted. This patch reverts waiting function to dumb
> >   Sleep().
>
> I understand the need for this change, but isn't there any other
> way to detect if the pseudo console being ready?  E. g., something
> in the HPCON_INTERNAL struct or so?

As for HPCON_INTERNAL,

typedef struct _HPCON_INTERNAL
{
  HANDLE hWritePipe;
  HANDLE hConDrvReference;
  HANDLE hConHostProcess;
} HPCON_INTERNAL;

hWritePipe:
This is for sending window size change message to pseudo console
(conhost.exe process).

hConDrvRererence:
I am not sure what this is for.

hConHostProcess:
Process handle of conhost.exe process.

None of them seems able to be used for that purpose.
I do not come up with other implementation so far.

Let me consider a while.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Koichi Murase
In reply to this post by Takashi Yano
2020年1月23日(木) 22:00 Takashi Yano <[hidden email]>:
> Is there any process alived using diffrent version of cygwin1.dll?

Ah, you were right!  Actually there were no *real* processes remained
(Otherwise I could not have overwritten cygwin1.dll, I think), but I
remembered that there is a remaining *fake entry* in the result of
`ps' as follows (for which `kill' produces error `No such process' and
also I cannot find any corresponding process in Windows Task Manager).

  $ ps uaxf
      PID    PPID    PGID     WINPID   TTY         UID    STIME COMMAND

     1416       1    1416      11160  ?         197610   Jan 20
  /home/murase/opt/screen/4.7.0m/bin/screen-4.7.0

After a reboot of Windows, the problem has resolved!  Thank you!

Koichi
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
In reply to this post by Corinna Vinschen-2
On Thu, 23 Jan 2020 13:51:54 +0100
Corinna Vinschen wrote:
> On Jan 23 13:30, Takashi Yano wrote:
> > - After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
> >   cygwin programs which call both printf() and WriteConsole() are
> >   frequently distorted. This patch reverts waiting function to dumb
> >   Sleep().
> I understand the need for this change, but isn't there any other
> way to detect if the pseudo console being ready?  E. g., something
> in the HPCON_INTERNAL struct or so?

In any case, this patch has a problem that windows native program takes
a very long time to start/stop after the window size is changed.

Please do not apply this patch.

If you want to try this patch, please use following patch along with
v2 patch.

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 2b7c667a6..06ac0c5e0 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2413,6 +2413,7 @@ fhandler_pty_master::ioctl (unsigned int cmd, void *arg)
   COORD size;
   size.X = ((struct winsize *) arg)->ws_col;
   size.Y = ((struct winsize *) arg)->ws_row;
+  get_ttyp ()->last_push_time = GetTickCount ();
   ResizePseudoConsole (get_pseudo_console (), size);
  }
       if (get_ttyp ()->winsize.ws_row != ((struct winsize *) arg)->ws_row


--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Corinna Vinschen-2
In reply to this post by Takashi Yano
On Jan 23 23:16, Takashi Yano wrote:

> On Thu, 23 Jan 2020 13:51:54 +0100
> Corinna Vinschen wrote:
> > On Jan 23 13:30, Takashi Yano wrote:
> > > - After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
> > >   cygwin programs which call both printf() and WriteConsole() are
> > >   frequently distorted. This patch reverts waiting function to dumb
> > >   Sleep().
> >
> > I understand the need for this change, but isn't there any other
> > way to detect if the pseudo console being ready?  E. g., something
> > in the HPCON_INTERNAL struct or so?
>
> As for HPCON_INTERNAL,
>
> typedef struct _HPCON_INTERNAL
> {
>   HANDLE hWritePipe;
>   HANDLE hConDrvReference;
>   HANDLE hConHostProcess;
> } HPCON_INTERNAL;
>
> hWritePipe:
> This is for sending window size change message to pseudo console
> (conhost.exe process).
>
> hConDrvRererence:
> I am not sure what this is for.
>
> hConHostProcess:
> Process handle of conhost.exe process.
>
> None of them seems able to be used for that purpose.
Too bad.  It's pretty strange that CreatePseudoConsole returns a
valid HPCON but then isn't ready to take input immediately.

> I do not come up with other implementation so far.
>
> Let me consider a while.

I wonder how others solve this problem.  I see that the native OpenSSH
is using Sleeps, too, in their start_with_pty() function, calling
AttachConsole in a loop, but I'm not sure if these are related to pseudo
console usage.  The commit message don't explain anything there :(


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
Hi Corinna,

On Fri, 24 Jan 2020 12:07:30 +0100
Corinna Vinschen wrote:

> Too bad.  It's pretty strange that CreatePseudoConsole returns a
> valid HPCON but then isn't ready to take input immediately.
>
> > I do not come up with other implementation so far.
> >
> > Let me consider a while.
>
> I wonder how others solve this problem.  I see that the native OpenSSH
> is using Sleeps, too, in their start_with_pty() function, calling
> AttachConsole in a loop, but I'm not sure if these are related to pseudo
> console usage.  The commit message don't explain anything there :(

The essence of the difficulty is that we have to support both cygwin
programs and native console apps. If we consider only of native console
apps, any time we can use pseudo console. However, pseudo console is
not transparent at all, so it cannot be used for cygwin programs.

Therefore, current cygwin is switching handles to be used between
named-pipe and pseudo console.

However, because pseudo console has relatively long latency, if pipe
is switched just after writing to pseudo console, the forwarding
does not get in time. So the "wait" is needed before switching.

I had tried WriteFile(), ReadFile() and DeviceIoControl() for
HANDLE hConDrvReference, however, all atempts of them failed.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
Hi Corinna,

On Sat, 25 Jan 2020 20:38:37 +0900
Takashi Yano wrote:

> On Fri, 24 Jan 2020 12:07:30 +0100
> Corinna Vinschen wrote:
> > Too bad.  It's pretty strange that CreatePseudoConsole returns a
> > valid HPCON but then isn't ready to take input immediately.
> >
> > > I do not come up with other implementation so far.
> > >
> > > Let me consider a while.
> >
> > I wonder how others solve this problem.  I see that the native OpenSSH
> > is using Sleeps, too, in their start_with_pty() function, calling
> > AttachConsole in a loop, but I'm not sure if these are related to pseudo
> > console usage.  The commit message don't explain anything there :(
>
> The essence of the difficulty is that we have to support both cygwin
> programs and native console apps. If we consider only of native console
> apps, any time we can use pseudo console. However, pseudo console is
> not transparent at all, so it cannot be used for cygwin programs.
>
> Therefore, current cygwin is switching handles to be used between
> named-pipe and pseudo console.
>
> However, because pseudo console has relatively long latency, if pipe
> is switched just after writing to pseudo console, the forwarding
> does not get in time. So the "wait" is needed before switching.
>
> I had tried WriteFile(), ReadFile() and DeviceIoControl() for
> HANDLE hConDrvReference, however, all atempts of them failed.

After much struggle, I finally found a solution.
Please look at v3 patch.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.

Takashi Yano
On Sun, 26 Jan 2020 22:33:19 +0900
Takashi Yano wrote:

> On Sat, 25 Jan 2020 20:38:37 +0900
> Takashi Yano wrote:
> > On Fri, 24 Jan 2020 12:07:30 +0100
> > Corinna Vinschen wrote:
> > > Too bad.  It's pretty strange that CreatePseudoConsole returns a
> > > valid HPCON but then isn't ready to take input immediately.
> > >
> > > > I do not come up with other implementation so far.
> > > >
> > > > Let me consider a while.
> > >
> > > I wonder how others solve this problem.  I see that the native OpenSSH
> > > is using Sleeps, too, in their start_with_pty() function, calling
> > > AttachConsole in a loop, but I'm not sure if these are related to pseudo
> > > console usage.  The commit message don't explain anything there :(
> >
> > The essence of the difficulty is that we have to support both cygwin
> > programs and native console apps. If we consider only of native console
> > apps, any time we can use pseudo console. However, pseudo console is
> > not transparent at all, so it cannot be used for cygwin programs.
> >
> > Therefore, current cygwin is switching handles to be used between
> > named-pipe and pseudo console.
> >
> > However, because pseudo console has relatively long latency, if pipe
> > is switched just after writing to pseudo console, the forwarding
> > does not get in time. So the "wait" is needed before switching.
> >
> > I had tried WriteFile(), ReadFile() and DeviceIoControl() for
> > HANDLE hConDrvReference, however, all atempts of them failed.
>
> After much struggle, I finally found a solution.
> Please look at v3 patch.

v3 patch does not seem to work as expected in Win10 1809.
I will submit v4 patch.

--
Takashi Yano <[hidden email]>
12