lambdalisue commented 6 years ago

ch_readraw() を mode:nl で利用した際に行ではない要素を取得できないため、全ての出力を取得するコードにて無限ループする問題があります。想定挙動をどうするのか？対処方法をどうするのか？に関して議論したほうが良いと思うので issue として上げます。

Vim

Vim 8.0.1322 on macOS High Sierra で確認を取っています。

再現方法

以下のように最後に改行文字を出力しないプロセスを起動した場合 :help read-in-close-cb にかかれているサンプルが無限ループします。

test.py

import sys
# foo\n
# \n
# bar\n
# \n
# hoge
sys.stdout.write("foo\n\nbar\n\nhoge")

test.vim

" :help read-in-close-cb
function! s:on_close(channel) abort
  echomsg 'Start'
  while ch_status(a:channel, {'part': 'out'}) ==# 'buffered'
    echomsg ch_read(a:channel)
  endwhile
  echomsg 'End'
endfunction

call job_start('python test.py', {
      \ 'out_mode': 'nl',
      \ 'close_cb': function('s:on_close'),
      \})

想定挙動

想定する挙動として、以下の二点が考えられます。

mode:nl の場合は行ではない最後の部分（例にて hoge）は一切取得できない
mode:nl の場合でも、これ以上データが来ないことが確定している場合（ジョブが停止、チャネルが閉じられている）は、データの最後に改行文字が存在していることを仮定する（最後の hoge が取れる）

どちらが良いのかは要議論だと思いますが、個人的には 2. のほうが全てのデータが取れてよいです。

対処方法

大きく分けて ch_canread() を修正する方法と ch_readraw() を修正する方法の二点があると思います。

`mode:nl` の際の `ch_canread()` の挙動を修正

ch_canread() の挙動が「（現在のモードにて）取得可能なデータがあるか？」と規定すると上記想定挙動の 1. として解決できます。具体的には以下のような挙動を想定しています。

echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'foo'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'bar'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 0
echo ch_readraw(channel)    " ''

チャネルの状態により `ch_readraw()` の挙動を修正

ジョブが停止やチャネルが閉じられている（閉じられようとしている）場合は ch_readraw() にて最後のデータも取得できるように修正すると 2. として解決できます。具体的には以下のような挙動を想定しています。

" チャネルが生きている場合
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'foo'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'bar'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
" ... 以下無限

" チャネルが閉じられている場合
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'foo'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'bar'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'hoge'
echo ch_canread(channel)    " 0
echo ch_readraw(channel)    " ''

`ch_readraw()` の `options` に `force` 的なものを足す

ch_readraw() に渡す options に force 的なものを足し force:1 の場合は改行文字を含んでいなくてもデータを返すように修正すると 2. として解決できます。具体的には以下のような挙動を想定しています。

echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'foo'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " 'bar'
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
echo ch_canread(channel)    " 1
echo ch_readraw(channel)    " ''
" ... 以下無限

let options = {'force': 1}
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " 'foo'
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " ''
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " 'bar'
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " ''
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " 'hoge'
echo ch_canread(channel, options)    " 0
echo ch_readraw(channel, options)    " ''

`ch_readraw()` の返り値を変える

ch_readraw() にてデータが読めない場合に v:null や 0 を返すように修正すると 1. として解決できます。後方互換性が失われるため現実的ではないですが options で obvious:1 が指定された場合のみなどにすればワンちゃんあるかも。具体的には以下のような挙動を想定しています。

let options = {'obvious': 1}
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " 'foo'
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " ''
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " 'bar'
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " ''
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " v:null
echo ch_canread(channel, options)    " 1
echo ch_readraw(channel, options)    " v:null
" ... 以下無限だが v:null が返ってきているため空行と区別ができ処理を止められる

mattn commented 6 years ago

個人的には、ch_canread や ch_read を修正するよりは ch_readraw の戻り値を分けるべきだと思いました。ch_canread は、まだ読まれていないブロックがあるかないかで 0/1 を返しています。また ch_read の mode:nl は改行コードが無いと値を返しません。mode:json も同じくパースできる状態でないと読み取りません。例えば ch_canread がデータの完全な状態、つまり nl であれば改行コードがある、JSON であればパース出来るという状態を見て、そうでない場合には 0 を返す事になるとすると、オーバーワークな気がしています。もしそれを可能にしたいのであれば ch_canread とは別の API が必要だと思います。

また ch_readraw のソースを見る限りですが、channel にひもづく fd が不正か、読み取りタイムアウトが発生した場合は明確にハンドリング出来るので、空行なのか読み取り失敗なのかを読み分ける事は可能だと思います。(確かではないですが)

ichizok commented 6 years ago

気になった点。現状では mode:nl のとき ch_read と ch_readraw は同じ動作です。これについて、:help ch_readraw は

ch_readraw({handle} [, {options}])                      ch_readraw()
                Like ch_read() but for a JS and JSON channel does not decode
                the message.  See channel-more.
                {only available when compiled with the +channel feature}

とあり、mode:nl での動作については言及してないため、矛盾はありません。

一方で、:help channel-more には

To read all output from a RAW channel that is available:
        let output = ch_readraw(channel)

ch_readraw を使うと mode:raw で読み出せる...という意味に取れます (?)

mattn commented 6 years ago

channel.c:3516 の部分の事でしょうか？ channel_read_block で raw であれば全部、nl かつ最初のチャンクが改行を含む場合だけチャンクから取り出すというコードになっていて、「動作は」違うように思います。

raw の処理、channel_get_all はもうチャンクがない場合には NULL を返します。これは期待通りです。

nl の場合は channel_read_block で改行が来ないのでタイムアウトし、チャンクを残したまま NULL が返ります。あとはスクリプトとして呼び出し元に、チャンクが残ってるかどうかを知れる仕組みがあれば良いかなと思っていています。

で、ここまで書いて ch_canread のソースを見たのですが、canread は単に channel が読みだし可能かだけしか返してないと思っていたのですが、JSON の場合だけちゃんとチャンクが JSON としてパース出来るかどうかを見ている様なので、意見を変えてしまう事になるのですが

    int
channel_has_readahead(channel_T *channel, ch_part_T part)
{
    ch_mode_T   ch_mode = channel->ch_part[part].ch_mode;

    if (ch_mode == MODE_JSON || ch_mode == MODE_JS)
    {
    jsonq_T   *head = &channel->ch_part[part].ch_json_head;
    jsonq_T   *item = head->jq_next;

    return item != NULL;
    }
    return channel_peek(channel, part) != NULL;
}

この channel_peek の呼び出しに、かつ mode:nl の場合は channel_first_nl かどうかを見てあげれば ch_canread が目的の動作をするのではないかと思い始めました。(sorry @thinca)

mattn commented 6 years ago

パッチで言うとこんな感じです。

diff --git a/src/channel.c b/src/channel.c
index 8fc705058..04f644f7a 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -2707,6 +2707,7 @@ channel_is_open(channel_T *channel)
 channel_has_readahead(channel_T *channel, ch_part_T part)
 {
     ch_mode_T  ch_mode = channel->ch_part[part].ch_mode;
+    cbq_T  *node;

     if (ch_mode == MODE_JSON || ch_mode == MODE_JS)
     {
@@ -2715,7 +2716,11 @@ channel_has_readahead(channel_T *channel, ch_part_T part)

    return item != NULL;
     }
-    return channel_peek(channel, part) != NULL;
+
+    node = channel_peek(channel, part);
+    if (ch_mode == NODE_NL)
+   return channel_first_nl(node);
+    return node != NULL;
 }

 /*

ichizok commented 6 years ago

channel_read_block で raw であれば全部、nl かつ最初のチャンクが改行を含む場合だけチャンクから取り出す

これ (raw/nl) は channel の mode ですよね？ f_ch_readraw から呼ばれる common_channel_read(argvars, rettv, raw) の raw は channel.c:3515 の if でしか使ってないので、channel_read_block の動作は ch_read と ch_readraw で変わらないと思ったんですが。

ichizok commented 6 years ago

ch_canread で part オプション受けられるようにすると、無指定時の振る舞いを他の channel 関数と変えざるを得ないのが微妙...

diff --git a/src/evalfunc.c b/src/evalfunc.c
index 76c576855..5509fd01c 100644
--- a/src/evalfunc.c
+++ b/src/evalfunc.c
@@ -521,7 +521,7 @@ static struct fst
     {"ceil",           1, 1, f_ceil},
 #endif
 #ifdef FEAT_JOB_CHANNEL
-    {"ch_canread",     1, 1, f_ch_canread},
+    {"ch_canread",     1, 2, f_ch_canread},
     {"ch_close",       1, 1, f_ch_close},
     {"ch_close_in",    1, 1, f_ch_close_in},
     {"ch_evalexpr",    2, 3, f_ch_evalexpr},
@@ -1850,12 +1850,27 @@ f_ceil(typval_T *argvars, typval_T *rettv)
 f_ch_canread(typval_T *argvars, typval_T *rettv)
 {
     channel_T *channel = get_channel_arg(&argvars[0], FALSE, FALSE, 0);
+    jobopt_T   opt;
+    int                part = -1;
+
+    if (argvars[1].v_type != VAR_UNKNOWN)
+    {
+       clear_job_options(&opt);
+       if (get_job_options(&argvars[1], &opt, JO_PART, 0) == OK
+                                                    && (opt.jo_set & JO_PART))
+           part = opt.jo_part;
+    }

     rettv->vval.v_number = 0;
     if (channel != NULL)
-       rettv->vval.v_number = channel_has_readahead(channel, PART_SOCK)
-                           || channel_has_readahead(channel, PART_OUT)
-                           || channel_has_readahead(channel, PART_ERR);
+    {
+       if (part == -1)
+           rettv->vval.v_number = channel_has_readahead(channel, PART_SOCK)
+                               || channel_has_readahead(channel, PART_OUT)
+                               || channel_has_readahead(channel, PART_ERR);
+       else
+           rettv->vval.v_number = channel_has_readahead(channel, part);
+    }
 }

 /*

mattn commented 6 years ago

ふむ。channel_read_block の中で再度 mode を取り直してるので、呼び出しは同じなのですが channel がどっちの mode かで動作が変わってしまう様ですね。なので readraw で読んでも channel が nl だと全部読み取れない事になってしまうんですね。ここに問題があるのかも。

channel_read_block に raw を渡してあげて、raw であれば mode が nl であっても全部読むってしないといけないのかな。

mattn commented 6 years ago

この件、まとめると問題が2つあって

ch_canread が JSON の場合はパース可能かどうかを判定しているのに nl の場合は改行を見ていない
ch_readraw が raw と言いながら channel の mode に依存しており nl の場合に全て読み取れていない

というのがあります。

diff --git a/src/channel.c b/src/channel.c
index 8fc705058..6e391a850 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -2707,6 +2707,7 @@ channel_is_open(channel_T *channel)
 channel_has_readahead(channel_T *channel, ch_part_T part)
 {
     ch_mode_T  ch_mode = channel->ch_part[part].ch_mode;
+    readq_T    *node;

     if (ch_mode == MODE_JSON || ch_mode == MODE_JS)
     {
@@ -2715,7 +2716,14 @@ channel_has_readahead(channel_T *channel, ch_part_T part)

    return item != NULL;
     }
-    return channel_peek(channel, part) != NULL;
+
+    node = channel_peek(channel, part);
+    if (node == NULL)
+   return FALSE;
+
+    if (ch_mode == MODE_NL)
+   return channel_first_nl(node) != NULL;
+    return TRUE;
 }

 /*
@@ -3316,8 +3324,8 @@ channel_read(channel_T *channel, ch_part_T part, char *func)
  * Returns what was read in allocated memory.
  * Returns NULL in case of error or timeout.
  */
-    char_u *
-channel_read_block(channel_T *channel, ch_part_T part, int timeout)
+    static char_u *
+channel_read_block(channel_T *channel, ch_part_T part, int timeout, int raw)
 {
     char_u *buf;
     char_u *msg;
@@ -3327,14 +3335,14 @@ channel_read_block(channel_T *channel, ch_part_T part, int timeout)
     readq_T    *node;

     ch_log(channel, "Blocking %s read, timeout: %d msec",
-                   mode == MODE_RAW ? "RAW" : "NL", timeout);
+               (raw || mode == MODE_RAW) ? "RAW" : "NL", timeout);

     while (TRUE)
     {
    node = channel_peek(channel, part);
    if (node != NULL)
    {
-       if (mode == MODE_RAW || (mode == MODE_NL
+       if (raw || mode == MODE_RAW || (mode == MODE_NL
                       && channel_first_nl(node) != NULL))
        /* got a complete message */
        break;
@@ -3354,7 +3362,7 @@ channel_read_block(channel_T *channel, ch_part_T part, int timeout)
     }

     /* We have a complete message now. */
-    if (mode == MODE_RAW)
+    if (raw || mode == MODE_RAW)
     {
    msg = channel_get_all(channel, part);
     }
@@ -3513,7 +3521,8 @@ common_channel_read(typval_T *argvars, typval_T *rettv, int raw)
        timeout = opt.jo_timeout;

    if (raw || mode == MODE_RAW || mode == MODE_NL)
-       rettv->vval.v_string = channel_read_block(channel, part, timeout);
+       rettv->vval.v_string = channel_read_block(channel, part,
+           timeout, raw);
    else
    {
        if (opt.jo_set & JO_ID)
@@ -3955,7 +3964,8 @@ ch_raw_common(typval_T *argvars, typval_T *rettv, int eval)
        timeout = opt.jo_timeout;
    else
        timeout = channel_get_timeout(channel, part_read);
-   rettv->vval.v_string = channel_read_block(channel, part_read, timeout);
+   rettv->vval.v_string = channel_read_block(channel, part_read,
+       timeout, TRUE);
     }
     free_job_options(&opt);
 }

これでどうでしょうか。

mattn commented 6 years ago

ん、なんかまずってますね。twitvim (channel使ってる)が動かない。

mattn commented 6 years ago

あ、canread の互換性を壊してしまうのか。むむむ。

mattn commented 6 years ago

https://gist.github.com/mattn/270362fa9a72dff143832ea0d150fc6f

このパッチであれば以下のスクリプトで全部読み取る事は出来ます。

" :help read-in-close-cb
function! s:on_close(channel) abort
  echomsg 'Start'
  while ch_status(a:channel, {'part': 'out'}) ==# 'buffered'
    echomsg ch_readraw(a:channel)
  endwhile
  echomsg 'End'
endfunction

call job_start('python test.py', {
      \ 'out_mode': 'nl',
      \ 'close_cb': function('s:on_close'),
      \})

※ ch_readraw に変えています。

ただ、いろいろ悩ましいのですが、例えばCの fgets とかだと改行無くても EOF が来ればそこまでは読み取ってくれるんですよね。ch_read (mode:nl) に手を入れたい気持ちもあります。。。どう思いますか？ @ichizok

ichizok commented 6 years ago

ただ、いろいろ悩ましいのですが、例えばCの fgets とかだと改行無くても EOF が来ればそこまでは読み取ってくれるんですよね。ch_read (mode:nl) に手を入れたい気持ちもあります。。。どう思いますか？ @ichizok

https://github.com/vim-jp/issues/issues/957#issuecomment-249137544 callback は mode:nl でも EOF まで読むようになってますし、ch_read もそれと揃えてよさそうに思います。

ichizok commented 6 years ago

ch_status(channel, {'part' : 'out', 'mode' : 'raw' }) のように mode 指定して状態をチェックできるようにするとか考えましたが、かえってややこしいか。これなら ch_rawstatus とかの方が良さそう

mattn commented 6 years ago

ch_read の結果には改行が含まれないので、呼び出し側は中途半端だったのか全部読み取ったのか分からないし、であればいっそ読み取らないで欲しい気もする。んー。悩ましい。。。

とは言え ch_readraw で全部読み取れる様にはなったので、あとは追加の API がいるかどうかを決めるべきですかね。ch_rawstatus だとして何を返しましょう？

ichizok commented 6 years ago

ch_read/ch_readraw と対応させて

ch_peek : (現在の mode で) 読めるものがあるか確認する
ch_peekraw : mode:raw として読めるものがあるか確認する

(ch_peek は https://github.com/vim-jp/issues/issues/1125#issuecomment-347551707 の ch_canread のような動作)

というのは？とにかく全て読みたいときは ch_peekraw/ch_readraw を使うと。

" in close_cb
let opt = {'part' : 'out'}
while ch_peek(ch, opt)
  echomsg ch_read(ch, opt)
endwhile
while ch_peekraw(ch, opt)
  echomsg ch_readraw(ch, opt)
endwhile

mattn commented 6 years ago

vim/vim で議論した方が良さそうな気がしますね。

mattn commented 6 years ago

https://github.com/vim/vim/issues/2425

mattn commented 6 years ago

Bram は ch_read で最後の NL のないデータを読めるのがお好みらしい。

h-east commented 6 years ago

8.0.1381 https://github.com/vim/vim/commit/620ca2da372dc9c892022faff83d363c67cc5c45

@lambdalisue close可能ならお願いします。

vim-jp / issues

ch_readraw と NL モードの問題提起 #1125

Vim

再現方法

想定挙動

対処方法

`mode:nl` の際の `ch_canread()` の挙動を修正

チャネルの状態により `ch_readraw()` の挙動を修正

`ch_readraw()` の `options` に `force` 的なものを足す

`ch_readraw()` の返り値を変える

vim-jp / issues

ch_readraw と NL モードの問題提起 #1125

Vim

再現方法

想定挙動

対処方法

mode:nl の際の ch_canread() の挙動を修正

チャネルの状態により ch_readraw() の挙動を修正

ch_readraw() の options に force 的なものを足す

ch_readraw() の返り値を変える

`mode:nl` の際の `ch_canread()` の挙動を修正

チャネルの状態により `ch_readraw()` の挙動を修正

`ch_readraw()` の `options` に `force` 的なものを足す

`ch_readraw()` の返り値を変える