New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lambdalisue opened this Issue Nov 28, 2017 · 19 comments

Comments

Projects
None yet
4 participants
@lambdalisue
Member

lambdalisue commented Nov 28, 2017

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'),
      \})

想定挙動

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

  1. mode:nl の場合は行ではない最後の部分(例にて hoge)は一切取得できない
  2. 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()optionsforce 的なものを足す

ch_readraw() に渡す optionsforce 的なものを足し 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:null0 を返すように修正すると 1. として解決できます。
後方互換性が失われるため現実的ではないですが optionsobvious: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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

個人的には、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 が不正か、読み取りタイムアウトが発生した場合は明確にハンドリング出来るので、空行なのか読み取り失敗なのかを読み分ける事は可能だと思います。(確かではないですが)

Member

mattn commented Nov 28, 2017

個人的には、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

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Nov 28, 2017

Member

気になった点。
現状では mode:nl のとき ch_readch_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 で読み出せる...という意味に取れます (?)

Member

ichizok commented Nov 28, 2017

気になった点。
現状では mode:nl のとき ch_readch_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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

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)

Member

mattn commented Nov 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

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

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;
 }
 
 /*
Member

mattn commented Nov 28, 2017

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

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

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Nov 28, 2017

Member

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_readch_readraw で変わらないと思ったんですが。

Member

ichizok commented Nov 28, 2017

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_readch_readraw で変わらないと思ったんですが。

@ichizok

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Nov 28, 2017

Member

ch_canreadpart オプション受けられるようにすると、無指定時の振る舞いを他の 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);
+    }
 }

 /*
Member

ichizok commented Nov 28, 2017

ch_canreadpart オプション受けられるようにすると、無指定時の振る舞いを他の 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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

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

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

Member

mattn commented Nov 28, 2017

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

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

この件、まとめると問題が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);
 }

これでどうでしょうか。

Member

mattn commented Nov 28, 2017

この件、まとめると問題が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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

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

Member

mattn commented Nov 28, 2017

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

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

Member

mattn commented Nov 28, 2017

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 28, 2017

Member

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

Member

mattn commented Nov 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Nov 28, 2017

Member

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

#957 (comment)
callback は mode:nl でも EOF まで読むようになってますし、ch_read もそれと揃えてよさそうに思います。

Member

ichizok commented Nov 28, 2017

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

#957 (comment)
callback は mode:nl でも EOF まで読むようになってますし、ch_read もそれと揃えてよさそうに思います。

@ichizok

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Nov 28, 2017

Member

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

Member

ichizok commented Nov 28, 2017

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 29, 2017

Member

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

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

Member

mattn commented Nov 29, 2017

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

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

@ichizok

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Dec 4, 2017

Member

ch_read/ch_readraw と対応させて

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

(ch_peek#1125 (comment)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
Member

ichizok commented Dec 4, 2017

ch_read/ch_readraw と対応させて

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

(ch_peek#1125 (comment)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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Dec 8, 2017

Member

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

Member

mattn commented Dec 8, 2017

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn
Member

mattn commented Dec 8, 2017

vim/vim#2425

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Dec 10, 2017

Member

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

Member

mattn commented Dec 10, 2017

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

@h-east

This comment has been minimized.

Show comment
Hide comment
@h-east

h-east Dec 23, 2017

Member

8.0.1381
vim/vim@620ca2d

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

Member

h-east commented Dec 23, 2017

8.0.1381
vim/vim@620ca2d

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment