Hi Jeremy,
Thanks for the updated patch and example.
The example below doesn't seem to deadlock for me however.
More eyes and review would be appreciated,
On Fri, Jun 28, 2024 at 07:46:27PM -0700, Jeremy Bobbin wrote:
> pasting a large thing into st can cause a deadlock
> commit 261ea4b attempted to fix this, but st still locks up when
> pasting over a million chars for example
> yes | head -c 1000000 | xclip -sel c
>
> this patch buffers writes to the TYY, which allows us to:
> - limit the size of writes to the TTY
> - conditionally poll for tty writability in the main loop
>
> in addition to simplifying the ttywrite function, this will allow you
> to paste very large things into st, while making make fewer calls to
> select should & may also make fewer calls to write.
> ---
> st.c | 90 +++++++++++++++++++++++++++---------------------------------
> st.h | 2 ++
> x.c | 10 +++++--
> 3 files changed, 51 insertions(+), 51 deletions(-)
>
> diff --git a/st.c b/st.c
> index 57c6e96..4fedeb8 100644
> --- a/st.c
> +++ b/st.c
> _AT_@ -152,6 +152,12 @@ typedef struct {
> int narg; /* nb of args */
> } STREscape;
>
> +typedef struct {
> + char *buf; /* allocated raw string */
> + size_t siz; /* allocation size */
> + size_t len; /* raw string length */
> +} TBuffer;
> +
> static void execsh(char *, char **);
> static void stty(char **);
> static void sigchld(int);
> _AT_@ -223,6 +229,7 @@ static Term term;
> static Selection sel;
> static CSIEscape csiescseq;
> static STREscape strescseq;
> +static TBuffer tbuf = { .buf = NULL, .siz = 0, .len = 0 };
> static int iofd = 1;
> static int cmdfd;
> static pid_t pid;
> _AT_@ -752,6 +759,30 @@ stty(char **args)
> perror("Couldn't call stty");
> }
>
> +size_t
> +tbuflen(void)
> +{
> + return tbuf.len;
> +}
> +
> +size_t
> +tbufwrite(void)
> +{
> + size_t lim = 256;
> + ssize_t r;
> +
> + /*
> + * Migrate the world to Plan 9.
> + */
> + if ((r = write(cmdfd, &tbuf.buf[0], MIN(tbuf.len, lim))) < 0)
> + die("write error on tty: %s\n", strerror(errno));
> +
> + tbuf.len -= r;
> + memmove(tbuf.buf, &tbuf.buf[r], tbuf.len);
> +
> + return r;
> +}
> +
> int
> ttynew(const char *line, char *cmd, const char *out, char **args)
> {
> _AT_@ -868,61 +899,22 @@ ttywrite(const char *s, size_t n, int may_echo)
> }
> }
>
> +
> void
> ttywriteraw(const char *s, size_t n)
> {
> - fd_set wfd, rfd;
> - ssize_t r;
> - size_t lim = 256;
> -
> - /*
> - * Remember that we are using a pty, which might be a modem line.
> - * Writing too much will clog the line. That's why we are doing this
> - * dance.
> - * FIXME: Migrate the world to Plan 9.
> - */
> - while (n > 0) {
> - FD_ZERO(&wfd);
> - FD_ZERO(&rfd);
> - FD_SET(cmdfd, &wfd);
> - FD_SET(cmdfd, &rfd);
> -
> - /* Check if we can write. */
> - if (pselect(cmdfd+1, &rfd, &wfd, NULL, NULL, NULL) < 0) {
> - if (errno == EINTR)
> - continue;
> - die("select failed: %s\n", strerror(errno));
> - }
> - if (FD_ISSET(cmdfd, &wfd)) {
> - /*
> - * Only write the bytes written by ttywrite() or the
> - * default of 256. This seems to be a reasonable value
> - * for a serial line. Bigger values might clog the I/O.
> - */
> - if ((r = write(cmdfd, s, (n < lim)? n : lim)) < 0)
> - goto write_error;
> - if (r < n) {
> - /*
> - * We weren't able to write out everything.
> - * This means the buffer is getting full
> - * again. Empty it.
> - */
> - if (n < lim)
> - lim = ttyread();
> - n -= r;
> - s += r;
> - } else {
> - /* All bytes have been written. */
> - break;
> - }
> - }
> - if (FD_ISSET(cmdfd, &rfd))
> - lim = ttyread();
> + if (tbuf.siz - tbuf.len < n) {
> + tbuf.siz = tbuf.siz ? tbuf.siz : 256;
> + while (tbuf.siz - tbuf.len < n)
> + tbuf.siz *= 2;
> + tbuf.buf = xrealloc(tbuf.buf, tbuf.siz);
> }
> +
> + memcpy(&tbuf.buf[tbuf.len], s, n);
> + tbuf.len += n;
> +
> return;
>
> -write_error:
> - die("write error on tty: %s\n", strerror(errno));
> }
>
> void
> diff --git a/st.h b/st.h
> index fd3b0d8..31d6fc4 100644
> --- a/st.h
> +++ b/st.h
> _AT_@ -87,6 +87,8 @@ void sendbreak(const Arg *);
> void toggleprinter(const Arg *);
>
> int tattrset(int);
> +size_t tbuflen(void);
> +size_t tbufwrite(void);
> void tnew(int, int);
> void tresize(int, int);
> void tsetdirtattr(int);
> diff --git a/x.c b/x.c
> index bd23686..e425ef3 100644
> --- a/x.c
> +++ b/x.c
> _AT_@ -1922,7 +1922,7 @@ run(void)
> {
> XEvent ev;
> int w = win.w, h = win.h;
> - fd_set rfd;
> + fd_set rfd, wfd;
> int xfd = XConnectionNumber(xw.dpy), ttyfd, xev, drawing;
> struct timespec seltv, *tv, now, lastblink, trigger;
> double timeout;
> _AT_@ -1948,8 +1948,11 @@ run(void)
>
> for (timeout = -1, drawing = 0, lastblink = (struct timespec){0};;) {
> FD_ZERO(&rfd);
> + FD_ZERO(&wfd);
> FD_SET(ttyfd, &rfd);
> FD_SET(xfd, &rfd);
> + if (tbuflen() > 0)
> + FD_SET(ttyfd, &wfd);
>
> if (XPending(xw.dpy))
> timeout = 0; /* existing events might not set xfd */
> _AT_@ -1958,7 +1961,7 @@ run(void)
> seltv.tv_nsec = 1E6 * (timeout - 1E3 * seltv.tv_sec);
> tv = timeout >= 0 ? &seltv : NULL;
>
> - if (pselect(MAX(xfd, ttyfd)+1, &rfd, NULL, NULL, tv, NULL) < 0) {
> + if (pselect(MAX(xfd, ttyfd)+1, &rfd, &wfd, NULL, tv, NULL) < 0) {
> if (errno == EINTR)
> continue;
> die("select failed: %s\n", strerror(errno));
> _AT_@ -1968,6 +1971,9 @@ run(void)
> if (FD_ISSET(ttyfd, &rfd))
> ttyread();
>
> + if (FD_ISSET(ttyfd, &wfd))
> + tbufwrite();
> +
> xev = 0;
> while (XPending(xw.dpy)) {
> xev = 1;
> --
> 2.45.2
>
>
--
Kind regards,
Hiltjo
Received on Sat Jun 29 2024 - 12:21:51 CEST