To: vim_dev@googlegroups.com Subject: Patch 8.0.0957 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ Patch 8.0.0957 Problem: When term_sendkeys() sends many keys it may get stuck in writing to the job. Solution: Make the write non-blocking, buffer keys to be sent. Files: src/terminal.c, src/channel.c, src/proto/channel.pro, src/structs.h src/testdir/test_terminal.vim *** ../vim-8.0.0956/src/terminal.c 2017-08-17 20:31:43.997203527 +0200 --- src/terminal.c 2017-08-17 22:04:56.705265387 +0200 *************** *** 400,405 **** --- 400,409 ---- vterm_get_size(term->tl_vterm, &term->tl_rows, &term->tl_cols); term_report_winsize(term, term->tl_rows, term->tl_cols); + /* Make sure we don't get stuck on sending keys to the job, it leads to + * a deadlock if the job is waiting for Vim to read. */ + channel_set_nonblock(term->tl_job->jv_channel, PART_IN); + if (old_curbuf != NULL) { --curbuf->b_nwindows; *** ../vim-8.0.0956/src/channel.c 2017-08-13 17:13:03.017546297 +0200 --- src/channel.c 2017-08-18 20:48:37.930214011 +0200 *************** *** 1373,1379 **** } /* ! * Write any lines to the input channel. */ static void channel_write_in(channel_T *channel) --- 1373,1379 ---- } /* ! * Write any buffer lines to the input channel. */ static void channel_write_in(channel_T *channel) *************** *** 1446,1451 **** --- 1446,1470 ---- } /* + * Write any lines waiting to be written to "channel". + */ + static void + channel_write_input(channel_T *channel) + { + chanpart_T *in_part = &channel->ch_part[PART_IN]; + + if (in_part->ch_writeque.wq_next != NULL) + channel_send(channel, PART_IN, (char_u *)"", 0, "channel_write_input"); + else if (in_part->ch_bufref.br_buf != NULL) + { + if (in_part->ch_buf_append) + channel_write_new_lines(in_part->ch_bufref.br_buf); + else + channel_write_in(channel); + } + } + + /* * Write any lines waiting to be written to a channel. */ void *************** *** 1454,1470 **** channel_T *channel; for (channel = first_channel; channel != NULL; channel = channel->ch_next) ! { ! chanpart_T *in_part = &channel->ch_part[PART_IN]; ! ! if (in_part->ch_bufref.br_buf != NULL) ! { ! if (in_part->ch_buf_append) ! channel_write_new_lines(in_part->ch_bufref.br_buf); ! else ! channel_write_in(channel); ! } ! } } /* --- 1473,1479 ---- channel_T *channel; for (channel = first_channel; channel != NULL; channel = channel->ch_next) ! channel_write_input(channel); } /* *************** *** 2984,2990 **** { chanpart_T *in_part = &ch->ch_part[PART_IN]; ! if (in_part->ch_fd != INVALID_FD && in_part->ch_bufref.br_buf != NULL) { FD_SET((int)in_part->ch_fd, wfds); if ((int)in_part->ch_fd >= maxfd) --- 2993,3001 ---- { chanpart_T *in_part = &ch->ch_part[PART_IN]; ! if (in_part->ch_fd != INVALID_FD ! && (in_part->ch_bufref.br_buf != NULL ! || in_part->ch_writeque.wq_next != NULL)) { FD_SET((int)in_part->ch_fd, wfds); if ((int)in_part->ch_fd >= maxfd) *************** *** 3530,3535 **** --- 3541,3571 ---- # endif /* + * Set "channel"/"part" to non-blocking. + */ + void + channel_set_nonblock(channel_T *channel, ch_part_T part) + { + chanpart_T *ch_part = &channel->ch_part[part]; + int fd = ch_part->ch_fd; + + if (fd != INVALID_FD) + { + #ifdef _WIN32 + if (part == PART_SOCK) + { + u_long val = 1; + + ioctlsocket(fd, FIONBIO, &val); + } + else + #endif + fcntl(fd, F_SETFL, O_NONBLOCK); + ch_part->ch_nonblocking = TRUE; + } + } + + /* * Write "buf" (NUL terminated string) to "channel"/"part". * When "fun" is not NULL an error message might be given. * Return FAIL or OK. *************** *** 3538,3551 **** channel_send( channel_T *channel, ch_part_T part, ! char_u *buf, ! int len, char *fun) { int res; sock_T fd; ! fd = channel->ch_part[part].ch_fd; if (fd == INVALID_FD) { if (!channel->ch_error && fun != NULL) --- 3574,3589 ---- channel_send( channel_T *channel, ch_part_T part, ! char_u *buf_arg, ! int len_arg, char *fun) { int res; sock_T fd; + chanpart_T *ch_part = &channel->ch_part[part]; + int did_use_queue = FALSE; ! fd = ch_part->ch_fd; if (fd == INVALID_FD) { if (!channel->ch_error && fun != NULL) *************** *** 3561,3589 **** { ch_log_lead("SEND ", channel); fprintf(log_fd, "'"); ! ignored = (int)fwrite(buf, len, 1, log_fd); fprintf(log_fd, "'\n"); fflush(log_fd); did_log_msg = TRUE; } ! if (part == PART_SOCK) ! res = sock_write(fd, (char *)buf, len); ! else ! res = fd_write(fd, (char *)buf, len); ! if (res != len) { ! if (!channel->ch_error && fun != NULL) { ! ch_error(channel, "%s(): write failed", fun); ! EMSG2(_("E631: %s(): write failed"), fun); } - channel->ch_error = TRUE; - return FAIL; - } ! channel->ch_error = FALSE; ! return OK; } /* --- 3599,3743 ---- { ch_log_lead("SEND ", channel); fprintf(log_fd, "'"); ! ignored = (int)fwrite(buf_arg, len_arg, 1, log_fd); fprintf(log_fd, "'\n"); fflush(log_fd); did_log_msg = TRUE; } ! for (;;) { ! writeq_T *wq = &ch_part->ch_writeque; ! char_u *buf; ! int len; ! ! if (wq->wq_next != NULL) ! { ! /* first write what was queued */ ! buf = wq->wq_next->wq_ga.ga_data; ! len = wq->wq_next->wq_ga.ga_len; ! did_use_queue = TRUE; ! } ! else { ! if (len_arg == 0) ! /* nothing to write, called from channel_select_check() */ ! return OK; ! buf = buf_arg; ! len = len_arg; } ! if (part == PART_SOCK) ! res = sock_write(fd, (char *)buf, len); ! else ! res = fd_write(fd, (char *)buf, len); ! if (res < 0 && (errno == EWOULDBLOCK ! #ifdef EAGAIN ! || errno == EAGAIN ! #endif ! )) ! res = 0; /* nothing got written */ ! ! if (res >= 0 && ch_part->ch_nonblocking) ! { ! writeq_T *entry = wq->wq_next; ! ! if (did_use_queue) ! ch_log(channel, "Sent %d bytes now", res); ! if (res == len) ! { ! /* Wrote all the buf[len] bytes. */ ! if (entry != NULL) ! { ! /* Remove the entry from the write queue. */ ! ga_clear(&entry->wq_ga); ! wq->wq_next = entry->wq_next; ! if (wq->wq_next == NULL) ! wq->wq_prev = NULL; ! else ! wq->wq_next->wq_prev = NULL; ! continue; ! } ! if (did_use_queue) ! ch_log(channel, "Write queue empty"); ! } ! else ! { ! /* Wrote only buf[res] bytes, can't write more now. */ ! if (entry != NULL) ! { ! if (res > 0) ! { ! /* Remove the bytes that were written. */ ! mch_memmove(entry->wq_ga.ga_data, ! (char *)entry->wq_ga.ga_data + res, ! len - res); ! entry->wq_ga.ga_len -= res; ! } ! buf = buf_arg; ! len = len_arg; ! } ! else ! { ! buf += res; ! len -= res; ! } ! ch_log(channel, "Adding %d bytes to the write queue", len); ! ! /* Append the not written bytes of the argument to the write ! * buffer. Limit entries to 4000 bytes. */ ! if (wq->wq_prev != NULL ! && wq->wq_prev->wq_ga.ga_len + len < 4000) ! { ! writeq_T *last = wq->wq_prev; ! ! /* append to the last entry */ ! if (ga_grow(&last->wq_ga, len) == OK) ! { ! mch_memmove((char *)last->wq_ga.ga_data ! + last->wq_ga.ga_len, ! buf, len); ! last->wq_ga.ga_len += len; ! } ! } ! else ! { ! writeq_T *last = (writeq_T *)alloc((int)sizeof(writeq_T)); ! ! if (last != NULL) ! { ! ch_log(channel, "Creating new entry"); ! last->wq_prev = wq->wq_prev; ! last->wq_next = NULL; ! if (wq->wq_prev == NULL) ! wq->wq_next = last; ! else ! wq->wq_prev->wq_next = last; ! wq->wq_prev = last; ! ga_init2(&last->wq_ga, 1, 1000); ! if (ga_grow(&last->wq_ga, len) == OK) ! { ! mch_memmove(last->wq_ga.ga_data, buf, len); ! last->wq_ga.ga_len = len; ! } ! } ! } ! } ! } ! else if (res != len) ! { ! if (!channel->ch_error && fun != NULL) ! { ! ch_error(channel, "%s(): write failed", fun); ! EMSG2(_("E631: %s(): write failed"), fun); ! } ! channel->ch_error = TRUE; ! return FAIL; ! } ! ! channel->ch_error = FALSE; ! return OK; ! } } /* *************** *** 3873,3885 **** if (ret > 0 && in_part->ch_fd != INVALID_FD && FD_ISSET(in_part->ch_fd, wfds)) { ! if (in_part->ch_buf_append) ! { ! if (in_part->ch_bufref.br_buf != NULL) ! channel_write_new_lines(in_part->ch_bufref.br_buf); ! } ! else ! channel_write_in(channel); --ret; } } --- 4027,4033 ---- if (ret > 0 && in_part->ch_fd != INVALID_FD && FD_ISSET(in_part->ch_fd, wfds)) { ! channel_write_input(channel); --ret; } } *** ../vim-8.0.0956/src/proto/channel.pro 2017-08-13 17:13:03.021546273 +0200 --- src/proto/channel.pro 2017-08-17 21:56:32.556321636 +0200 *************** *** 35,40 **** --- 35,41 ---- void common_channel_read(typval_T *argvars, typval_T *rettv, int raw); channel_T *channel_fd2channel(sock_T fd, ch_part_T *partp); void channel_handle_events(void); + void channel_set_nonblock(channel_T *channel, ch_part_T part); int channel_send(channel_T *channel, ch_part_T part, char_u *buf, int len, char *fun); void ch_expr_common(typval_T *argvars, typval_T *rettv, int eval); void ch_raw_common(typval_T *argvars, typval_T *rettv, int eval); *** ../vim-8.0.0956/src/structs.h 2017-08-13 20:28:48.955421362 +0200 --- src/structs.h 2017-08-17 22:18:02.088521879 +0200 *************** *** 1196,1201 **** --- 1196,1202 ---- typedef struct jobvar_S job_T; typedef struct readq_S readq_T; + typedef struct writeq_S writeq_T; typedef struct jsonq_S jsonq_T; typedef struct cbq_S cbq_T; typedef struct channel_S channel_T; *************** *** 1512,1517 **** --- 1513,1525 ---- readq_T *rq_prev; }; + struct writeq_S + { + garray_T wq_ga; + writeq_T *wq_next; + writeq_T *wq_prev; + }; + struct jsonq_S { typval_T *jq_value; *************** *** 1601,1606 **** --- 1609,1616 ---- #endif int ch_block_write; /* for testing: 0 when not used, -1 when write * does not block, 1 simulate blocking */ + int ch_nonblocking; /* write() is non-blocking */ + writeq_T ch_writeque; /* header for write queue */ cbq_T ch_cb_head; /* dummy node for per-request callbacks */ char_u *ch_callback; /* call when a msg is not handled */ *** ../vim-8.0.0956/src/testdir/test_terminal.vim 2017-08-14 23:07:26.952471391 +0200 --- src/testdir/test_terminal.vim 2017-08-17 22:04:11.813538134 +0200 *************** *** 450,452 **** --- 450,465 ---- exe buf . 'bwipe!' call assert_equal("", bufname(buf)) endfunction + + func Test_terminal_noblock() + let buf = term_start(&shell) + + for c in ['a','b','c','d','e','f','g','h','i','j','k'] + call term_sendkeys(buf, 'echo ' . repeat(c, 5000) . "\") + endfor + + let g:job = term_getjob(buf) + call Stop_shell_in_terminal(buf) + call term_wait(buf) + bwipe + endfunc *** ../vim-8.0.0956/src/version.c 2017-08-17 20:31:44.001203501 +0200 --- src/version.c 2017-08-18 20:41:18.189042840 +0200 *************** *** 771,772 **** --- 771,774 ---- { /* Add new patch number below this line */ + /**/ + 957, /**/ -- DENNIS: Look, strange women lying on their backs in ponds handing out swords ... that's no basis for a system of government. Supreme executive power derives from a mandate from the masses, not from some farcical aquatic ceremony. "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///