[patch] 2 problems about job
Ozaki Kiichi
There are 2 problems about job-channel callback.
1) callback isn't invoked when last line doesn't terminate by NL.
[repro steps]
test.vim
---
function! Callback(ch, msg)
echom a:msg
let g:linecount += 1
endfunction
let g:linecount = 0
call job_start(['python', '-c', 'import sys;sys.stdout.write("1\n2\n3")'], {'callback': 'Callback'})
---
vim -Nu NONE -S test.vim
expected: g:linecount == 3 and ':messages shows;
----
1
2
3
----
actual: g:linecount == 2 and ':messages' shows;
----
1
2
----
[patch]
- Flush last line when no ending NL
https://gist.github.com/ichizok/a60b96fc5c913b260c34a7e9e1a02de8
2) callback isn't invoked when job immediately terminates.
[repro steps]
test.vim
----
function! Callback(ch, msg)
echom a:msg
let g:linecount += 1
endfunction
let g:linecount = 0
call job_start(['python', '-c', 'import os,sys;os.close(1);sys.stderr.write("1\n")'], {'callback': 'Callback'})
----
vim -Nu NONE -S test.vim
expected: ':messages' shows "1" and g:linecount == 1
actual: ':messages' shows nothing and g:linecount == 0
[cause]
When one of the fds associated with job-channel reachs EOF,
job-channel are closed even if other fds has readable data.
[patch]
- Don't close job-channel until all readable fds are closed
https://gist.github.com/ichizok/6e0c00daf387b32bebcc2972f0cca137
Thank you.
- Ozaki Kiichi
mattn
>
> https://gist.github.com/ichizok/6e0c00daf387b32bebcc2972f0cca137
Bram, in this patch, you can see workaround to avoid a bug with escaping double-quote on Windows.
I'm working this in separately. I'll report a description and send patch in later.
Thanks.
Bram Moolenaar
there was just a discussion about systemlist() making a difference for a
missing NL at the end. But since the channel is in NL mode it's at
least not expected to drop text if the final NL is missing (or there is
no NL at all). One can use raw mode if the difference matters.
> 2) callback isn't invoked when job immediately terminates.
>
> [repro steps]
>
> test.vim
>
> ----
> function! Callback(ch, msg)
> echom a:msg
> let g:linecount += 1
> endfunction
>
> let g:linecount = 0
> call job_start(['python', '-c', 'import os,sys;os.close(1);sys.stderr.write("1\n")'], {'callback': 'Callback'})
> ----
>
> vim -Nu NONE -S test.vim
>
> expected: ':messages' shows "1" and g:linecount == 1
>
> actual: ':messages' shows nothing and g:linecount == 0
>
> [cause]
>
> When one of the fds associated with job-channel reachs EOF,
> job-channel are closed even if other fds has readable data.
>
> [patch]
>
> - Don't close job-channel until all readable fds are closed
>
> https://gist.github.com/ichizok/6e0c00daf387b32bebcc2972f0cca137
Doing this with reference counting is tricky. It looks like if in
channel_set_pipes() the may_close_part() actually closes an fd, then
ch_to_be_closed is not decremented. Also, when debugging it's hard to
see what happened. I think it would work to have a bitmask: Set the
bit for the part that's opened, reset it when closed or when an error
was detected on the part. When the bitmask is zero we are done with the
channel.
The message in channel_close_on_error() that the read failed should
happen on every read fail. Thus if there is an error on stdout but
stderr is still readable, it should log "cannot read from stdout".
Otherwise we don't know what is happening.
--
MARTHA'S WAY: Don't throw out all that leftover wine. Freeze into ice cubes
for future use in casseroles and sauces.
MY WAY: What leftover wine?
/// Bram Moolenaar -- Br...@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 ///
Bram Moolenaar
Checking for Win32 might not be right when 'shell' is not cmd.exe but a
Unix-shell-like.
--
BLACK KNIGHT: Come on you pansy!
[hah] [parry thrust]
[ARTHUR chops the BLACK KNIGHT's right arm off]
ARTHUR: Victory is mine! [kneeling]
We thank thee Lord, that in thy merc-
[Black Knight kicks Arthur in the head while he is praying]
The Quest for the Holy Grail (Monty Python)
mattn
> Yasuhiro Matsumoto wrote:
>
> > On Monday, October 3, 2016 at 7:49:20 AM UTC+9, Ozaki Kiichi wrote:
> > >
> > > https://gist.github.com/ichizok/6e0c00daf387b32bebcc2972f0cca137
> >
> > Bram, in this patch, you can see workaround to avoid a bug with escaping double-quote on Windows.
> >
> > https://gist.github.com/ichizok/6e0c00daf387b32bebcc2972f0cca137#file-fix-job-channel-patch-L202-L207
> >
> > I'm working this in separately. I'll report a description and send patch in later.
>
> Thanks. Escaping is tricky. Please check a few different shells.
> Checking for Win32 might not be right when 'shell' is not cmd.exe but a
> Unix-shell-like.
No need to check shell(s) because job_start doesn't use shell to start process. And the problem occur only on Windows.
shellexecute() make dup for double-quote like below.
shellescape('"') => """" (This mean " and "" and " )
This is right behavior when using cmd.exe. But not right for starting process without cmd.exe. Below is a test program.
foo.c
------------
#include <windows.h>
int
main(int argc, char* argv[]) {
MessageBox(0, argv[1], "foo", MB_OK);
return 0;
}
------------
C:\>foo.exe """"
This should work because it runs on cmd.exe. But it doesn't work when starting process without cmd.exe. Please try following.
------------
#include <windows.h>
#include <stdio.h>
int
main(int argc, char* argv[]) {
PROCESS_INFORMATION pi;
STARTUPINFO si;
ZeroMemory(&si, sizeof(STARTUPINFO));
CreateProcess(NULL, "foo \"import sys;sys.stdout.write(""1\\n2\\n3"")\"", NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi);
WaitForSingleObject(pi.hProcess, INFINITE);
}
------------
You expect this to display quoted "1\n2\n3" but this display non-quoted 1\n2\n3.
http://go-gyazo.appspot.com/949aca13dc74e8c3.png
See this article. https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
We should escape double-quote as \" instead of "" if we run without cmd.exe, so we MUST NOT use shellescape for this. Below is an implementation of this escaping.
https://gist.github.com/d47e7d3bfe5ade4be86062b565a4bfca
This change will be tested via Test_read_nonl_line.
Thanks.
- mattn
Bram Moolenaar
it's quite different from what we have before.
I think the "escaping" value needs to be reset after
for (i = 0; i < escaping; i++)
Looks like you get there with a sequence \""
--
The early bird gets the worm. If you want something else for
breakfast, get up later.
mattn
> Thanks for diving into this. But I think this needs more testing, since
> it's quite different from what we have before.
>
> I think the "escaping" value needs to be reset after
> for (i = 0; i < escaping; i++)
> Looks like you get there with a sequence \""
No, \n"" should be \\n\"\". i.e. second double-quote should be \". I'll add test in later.
mattn
Ozaki Kiichi
> channel_set_pipes() the may_close_part() actually closes an fd, then
> ch_to_be_closed is not decremented. Also, when debugging it's hard to
> see what happened. I think it would work to have a bitmask: Set the
> bit for the part that's opened, reset it when closed or when an error
> was detected on the part. When the bitmask is zero we are done with the
> channel.
>
> The message in channel_close_on_error() that the read failed should
> happen on every read fail. Thus if there is an error on stdout but
> stderr is still readable, it should log "cannot read from stdout".
> Otherwise we don't know what is happening.
Thank you. I updated patch.
https://gist.github.com/ichizok/6e0c00daf387b32bebcc2972f0cca137
- handle "ch_to_be_closed" as bitset.
- modified "ch_close_part_on_error()" logging, shows reason (EOF or error).
Ozaki Kiichi
I'm wondering:
In channel_close_on_error, when there is netbeans channel, channel_save writes to PART_OUT, but is this right? to PART_SOCK?
Bram Moolenaar
I'll change it. No test fails, don't really know how to test for this.
--
ARTHUR: Did you say shrubberies?
ROGER: Yes. Shrubberies are my trade. I am a shrubber. My name is Roger
the Shrubber. I arrange, design, and sell shrubberies.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
Bram Moolenaar
+strsave_for_argv(char_u *string)
I would call the argument "argv" and the escaped result "argv_escaped".
+ has_spaces = 1;
Use TRUE and FALSE instead of 1 and 0.
+ if (*p == '\n')
+ ++length;
But the "\n" is not escaped, thus this is not needed.
+ if (escaped_string == NULL)
+ return escaped_string;
Can return NULL.
+ else
+ if (*p == '\\')
Should be one line.
Also, there is no test for a backslash in the text. It would also need
to test for two or three backslashes. And one or two backslashes
followed by a double quote.
I would rename "p" to "s" (stands for source).
+ /* add terminating quote and finish with a NUL */
+ if (has_spaces)
+ {
+ for (i = 0; i < escaping; i++)
+ *d++ = '\\';
+ *d++ = '"';
+ }
+ *d = NUL;
Adding backslashes before the terminating quote looks strange. If this
is correct it should be covered by a test.
+func s:test_list_args(msg, out)
Use "cmd" instead of "msg".
+ call job_start([s:python, '-c', a:msg], {'callback': {ch,msg->execute('let s:out.=msg."\n"')}})
Why append the "\n"?
+ call s:test_list_args('print(''hello"world'')', "hello\"world\n")
This seems wrong. The argument is hello"world but it results in an
extra backslash.
--
BROTHER MAYNARD: Armaments Chapter Two Verses Nine to Twenty One.
ANOTHER MONK: And St. Attila raised his hand grenade up on high saying "O
Lord bless this thy hand grenade that with it thou mayest
blow thine enemies to tiny bits, in thy mercy. "and the Lord
did grin and people did feast upon the lambs and sloths and
carp and anchovies and orang-utans and breakfast cereals and
fruit bats and...
BROTHER MAYNARD: Skip a bit brother ...
mattn
https://gist.github.com/mattn/d47e7d3bfe5ade4be86062b565a4bfca
On Wednesday, October 12, 2016 at 11:19:06 PM UTC+9, Bram Moolenaar wrote:
> +strsave_for_argv(char_u *string)
>
> I would call the argument "argv" and the escaped result "argv_escaped".
renamed strsave_for_argv to escape_arg, string to arg, escaped_string to arg_escaped.
>
> + has_spaces = 1;
>
> Use TRUE and FALSE instead of 1 and 0.
okay, thank you.
> + if (*p == '\n')
> + ++length;
>
> But the "\n" is not escaped, thus this is not needed.
Yes, removed.
> + if (escaped_string == NULL)
> + return escaped_string;
>
> Can return NULL.
this too.
> + else
> + if (*p == '\\')
>
> Should be one line.
ditto
> Also, there is no test for a backslash in the text. It would also need
> to test for two or three backslashes. And one or two backslashes
> followed by a double quote.
>
> I would rename "p" to "s" (stands for source).
ditto
> + /* add terminating quote and finish with a NUL */
> + if (has_spaces)
> + {
> + for (i = 0; i < escaping; i++)
> + *d++ = '\\';
> + *d++ = '"';
> + }
> + *d = NUL;
>
> Adding backslashes before the terminating quote looks strange. If this
> is correct it should be covered by a test.
Yes, I was confused. Totally, fixed implementation. But terminating quote is right. See test code.
> +func s:test_list_args(msg, out)
>
> Use "cmd" instead of "msg".
fixed too
> + call job_start([s:python, '-c', a:msg], {'callback': {ch,msg->execute('let s:out.=msg."\n"')}})
>
> Why append the "\n"?
For the separator, but not required at least. Then removed.
> + call s:test_list_args('print(''hello"world'')', "hello\"world\n")
>
> This seems wrong. The argument is hello"world but it results in an
> extra backslash.
Please look carefully. "hello\"world\n"
Double quote in the quote should be escaped with backslash on Vim script as you know.
Thanks.
- mattn
Bram Moolenaar
--
ARTHUR: No, hang on! Just answer the five questions ...
GALAHAD: Three questions ...
ARTHUR: Three questions ... And we shall watch ... and pray.