Am 27.10.2016 um 10:47 schrieb Johannes:

Hello,

following you can find my little review of the process library. Unfortunately, I have no more time to dive into more details of the library. But from what I have read and seen on the list I think that the library would be a good addition to boost. The author seems to be quite knowledgeable about the library domain and I think that he will bring it into the state that will be consensus after the review.

...

- How much effort did you put into your evaluation of the review? About two hours

Following the notes I have taken during studying the tutorial: Spelling error: 'Wwe' in Tutorial/Starting a process

Sentence: 'Now, there is a subtle difference between the two syntaxes, i.e. passing a single string or passing one.' Should be '..., i.e. passing a single string or passing several strings.'

Puzzling: '... when passing one string it will be interpreted as a command. This might lead to different results which are platform dependent.' Could you elaborate? Yeah I should, especially because it's platform dependent. There's a bit more doc to that here: http://klemens-morgenstern.github.io/process/boost_process/design.html#boost...

Thank you for your review. It was surely a mistake to only read the sources of the doc for errors, so I missed too many things; I'm really confused why so many links didn't work. I fear I cannot change the documentation during the review, but you're certainly right, it is a weak point. Nevertheless, I'll go through and address your points, so other reviewers might have some questions answered. Despite the sloppy errors in the doc, it really helps me, because I am obviously very deep in the topic, so I don't always knwo what needs explaining. though that doesn't explain the differences in the implementation.

Unclear: 'The call to wait is necessary, to obtain it and tell the operating system, that no one is waiting for the process anymore.' What is meant by 'it' in '..., to obtain it ...'?

The exit code.

Unclear: 'As with all other functions, passing an std::error_code will change the behaviour, so that ...' Whar are all these other functions? child::child(), system, launch

Preferable: I would like to see bp::null_device instead of bp::null

Hmmm: With respect to 'bp::system("g++", "main.cpp", bp::std_out > "gcc_out.log");' In the reference http://klemens-morgenstern.github.io/process/boost/process/std_out.html you write: 'The Semantic is the same as for std_out' Do you mean std::cout or unix shell standard output? No I mean std_err, my bad.

The syntax is a little foreign to me. Actually, I would have expected something in the line of... 'bp::system("g++", "main.cpp") > bp::null_device;' 'bp::system("g++", "main.cpp") > "gcc_out.log"' or 'bp::system("g++", "main.cpp", bp::null_device);' 'bp::system("g++", "main.cpp", "gcc_out.log");' or 'bp::system("g++", "main.cpp", bp::std_out = bp::null_device);' 'bp::system("g++", "main.cpp", bp::std_out = "gcc_out.log");'

At least, you could elaborate somewhere, why you choose your syntax. It is not self explaining and should be introduced in more detail.

The latter actually works ( with bp::null that is), and it's because you redirect one stream. You of course have to pass it to system, because you have to set this all up before launching the process. But sure, that should be explained.

Example: 'std::vectorstd::string read_outline(std::string & file)' I did miss the c.wait() call. Is it not necessary here and if so why?

Well it is actually not necessary because the function "running" takes care of that. But that is in fact not clearly documented, I'll do that.

Unclear: '...and with [globalname boost::process::std_out std_out] by using [classname boost::process::opstream opstream]]' What are these?

Because of knowledge of tool 'nm' I do not understand the pipe example 'bp::child c("c++filt", std_out > out, std_in < in);' Thanks for the remark; I actually assumed every C++ developer would know

A fragment of a sentence I some how missed. the gnu toolchain, which would make this the perfect example. Nm reads the outline (i.e. every entry point) of a binary and prints them into stdout.

Unclear: You told me about child::wait and its variants and mentioned child::detach but now you use child::terminate. I wished you would explain these functions in the tutorial and not expect me the look up the reference here. Actually, the reference does not really tell me anything: 'Terminate the child process.' What does process termination mean. What is the state of the child instance that I have at hand? Can I asked if a child is actually terminated?

Hmm: Another unknown tool 'c++filt'. Please introduce these tools a little more. Not everyone has an unix toolset background.

It takes a mangled name and outputs the demangled name.

Example: Again you are not working with child::wait anymore.

Unclear: In the example 'std::vectorstd::string read_demangled_outline(std::string & file)' you introduce the class 'pipe'. and use it in 'bp::child nm("nm", bp::std_out > p);'. After what I have read until know in the tutorial I did have expected that you would come up with a iopstream object in analogon to the std::iostream. Give some background to the 'pipe' class for sake of clarification. You have room in the tutorial to take your customers at hand. I see, yes I assumed the knowledge of what a pipe is. A pstream wraps aronud a pipe, but since p is only used to forward it, it doesn't need a stream around it. I'll add more information there.

Example: Async example with std::future. That example is not self explaining. 'bp::std_in.close()' ? 'bp::std_out > bp::null, //so it can be written without anything' ? 'bp::std_err > data' ? A'm I only interested in errors in this example?

Please give more room for explanations. You are selling your library in the tutorial.

Sure thing, or at the very least link to the reference.

Unclear: 'This will also apply for a child process, that launches other processes, if they do not modifiy the group membership. ...' I did not understand your sentence here. You need to teach more about groups and what can be done with them.

Yes, and I'll add links to the actual implementation, which are job objects on windows and process groups on posix.

Unclear: 'Stackless coroutines can be implemented rather easily, so...'. So you are expecting from me in the tutorial that I'm familiar with the coroutines concept. No I'm not. Please take your time to provide me with enough background information so that I might be willing to invest resources to tackle the coroutine concepts and use it with your process library.

Here I do in fact assume prior knowledge, that's why it's linked to the boost.asio coroutine stuff.

Best, Johannes

Liebe Grüße, Klemens

Am 27.10.2016 um 21:27 schrieb Antony Polukhin:
> 2016-10-27 20:41 GMT+03:00 Klemens Morgenstern :
>> Am 27.10.2016 um 10:47 schrieb Johannes:
>>> Hello,
>>>
>>> following you can find my little review of the process library.
>>> Unfortunately, I have no more time to dive into more details of the library.
>>> But from what I have read and seen on the list I think that the library
>>> would be a good addition to boost. The author seems to be quite
>>> knowledgeable about the library domain and I think that he will  bring it
>>> into the state that will be consensus after the review.
>>>
>> Thank you for your review. It was surely a mistake to only read the sources
>> of the doc for errors, so I missed too many things; I'm really confused why
>> so many links didn't work. I fear I cannot change the documentation during
>> the review, but you're certainly right, it is a weak point. Nevertheless,
>> I'll go through and address your points, so other reviewers might have some
>> questions answered.
> It is allowed to fix links/typos in the docs during review. Just make sure that:
> * the docs are always available
> * that sections do not change their names and addresses
>
> In that way people will get the fixed docs and links to sections will
> remain valid during the whole discussion.
>
Awesome, I updated the documentation, and fixed a lot of typos.
I did not yet add detailed descriptions for nm, c++filt and coroutines, 
but I linked to their documentation. I'll gladly write more 
documentation after the review, since hastily adding them now would only 
result in more errors and confusion.

On 27 Oct 2016 at 20:33, Klemens Morgenstern wrote:

Well for the reason statet above I think this API design is the best solution; it has very little to do with backwards-compatibility, I broke nearly everything there. But as statet in our last discussion: I'd be really interested in how you would design that.

Something like https://godbolt.org/g/38C0KH maybe. The point I'm trying to make is that a child process class ought to be as primitive as possible. Absolutely build *layers* of additional fancy stuff on top. But the core base class should be something simple, easily subclassable to extend by users. Get the basics right first, THEN add the unnecessary stuff on top. That way you don't encumber your users with stuff they don't want nor need. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

Am 01.11.2016 um 00:15 schrieb Gavin Lambert:

On 28/10/2016 07:33, Klemens Morgenstern wrote:

...

...

6. It is a *very* serious design flaw that i/o deadlocks if you try to read after the child exits. The documentation suggests this is because maybe because we are redirecting to a file. This is not a good reason to have i/o deadlocking seeing as every system I've ever used it is possible to configure the pipes to error if you try to read from a closed pipe. The default should be to always error on a closed pipe read, and if the user is foolish enough to redirect to a file then it's on them to work around it rather than forcing very unhelpful and bug inducing behaviour on every user.

That is not the reason. In order to use boost.asio with pipes, you need to use named pipes on windows, which behave like file-handles. Therefore (afaik) the pipe does not get closed automatically, hence I cannot guarantee that a pipe will be closed automatically. The deadlock is caused for that reason: the pipe is still open, but no one is writing into it.

+1 for deadlock on read from exited process being a non-starter. (The only acceptable deadlock is for obviously silly user code, eg. synchronous blocking on reading the child's stdout without providing sufficient stdin to it. But trying to read stdout after the child has closed is not one of those cases.)

I do agree, but I cannot consistently guarantee that. The only way to ensure that would be something like FD_CLOEXEC, but there's no such thing on windows.

I don't really understand your explanation. File handles do get closed automatically on process exit, so as long as the child process is the only one that has the write-end of the pipe open, then the read-end should be guaranteed to EOF when the child exits on all platforms.

Not if you have a named pipe, that's the problem here. It could of course be that I missed something, but as far as I tested, you won't have a guaranteed close there.

If the pipe handles are being inherited by the child process (as with fork, or explicit in CreateProcess) then you should set parent-only handles to be non-inheritable prior to forking and close child-only handles in the parent immediately after forking.

Well as for the first: I don't know if it's parent-only in the function, since forwarding is possible. For the second: yes I could assume that a pipe will not be reused and close the half, but that wouldn't do anything for the problem at hand as explained above.

Also, you shouldn't have to use named pipes. CreatePipe will create anonymous pipes, which are just handles, and ASIO supports read/write operations on arbitrary handles just fine. For launching child processes this should be strongly preferred over named pipes.

It does not support that, you'll get an exception if you try it. THAT is the main problem. In order to make it asynchronous, you need the overlapped stuff, and you cannot do that with anonymous pipes. That is the main reason this doesn't work, becaues I have to assume that any given pipe may be a named one.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Am 01.11.2016 um 01:17 schrieb Gavin Lambert:

On 1/11/2016 13:00, Klemens Morgenstern wrote:

...

Not if you have a named pipe, that's the problem here. It could of course be that I missed something, but as far as I tested, you won't have a guaranteed close there.

It should; they're still just handles after all. The pipe server itself is multi-use but the individual read/write pipe handles aren't.

Not sure there, I don't think that's entirely correct. At least from what I tested. The closing of one side only does only work for anonymous pipes afaik.

Note that async_pipe has the same move-constructor bug that basic_pipe does (and that I made a PR for). Perhaps that's the reason that you had the issue, since if you ever move-constructed it you'd leave a dangling unclosed handle.

Thanks, just fixed that. I don't think that's the reason I had the same issue with process 0.5.

...

Well as for the first: I don't know if it's parent-only in the function, since forwarding is possible. For the second: yes I could assume that a pipe will not be reused and close the half, but that wouldn't do anything for the problem at hand as explained above.

If you're creating the pipes yourself for the express purpose of communicating with the child, then it should be well-defined which end of the pipe each process will be using, and you should be able to ensure that only one end is open in each process. No, I am not. Since you have streams, they must have a handle so I need to be able to duplicate pipes. Hence I could just close a duplicate, which doesn't do a thing to help with the problem. I really looked for a consistent solution, but sadly I didn't find one. That's why I invested so much effort in making the asio-interface easy. It could of course be that I missed something, but I didn't find a solution that would be the same for the user on windows as on linux.

...

It does not support that, you'll get an exception if you try it. THAT is the main problem. In order to make it asynchronous, you need the overlapped stuff, and you cannot do that with anonymous pipes. That is the main reason this doesn't work, becaues I have to assume that any given pipe may be a named one.

True, I forgot about the overlapped I/O issue. That does complicate things, but as long as you use a guaranteed-unique name then you should be able to get equivalent behaviour. That's what's happening if you use a named pipe, it will create a unique name.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Am 01.11.2016 um 08:15 schrieb Gavin Lambert:

On 1/11/2016 13:34, Klemens Morgenstern wrote:

...

No, I am not. Since you have streams, they must have a handle so I need to be able to duplicate pipes. Hence I could just close a duplicate, which doesn't do a thing to help with the problem.

If you're creating a stream around the handle then the stream should own the handle -- you can duplicate it if you need to (eg. to change inheritance or security) but then close the original. Or to put it a different way, the handle should be moved into the stream, not copied.

It's generally a bad idea to have more than one handle pointing at the same thing. It's unavoidable for design reasons as given in this case:

pipe p; opstream os(p); ipstream is(p); os << 42; int i; is >> i; And I cannot only pass half of the pipe to opstream or ipstream, because: pipe p; ipstream is(p); child c("foo", std_out> is);//here I need the handle

So for stdout for example, you get a "read handle" and a "write handle" when the pipe is constructed. You move the read handle to the ipstream, and the write handle to the STARTUPINFO; after CreateProcess returns (and before you try to use the read handle) you close the write handle. Now the only handle to the pipe that the parent has is the read handle in the ipstream. No duplication.

Here is some very dumb code that demonstrates pipes working as expected:

LPCWSTR name = L"\\\\.\\pipe\\testpipe"; HANDLE hRead = CreateNamedPipeW(name, PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE | FILE_FLAG_OVERLAPPED, PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, 1, 0, 0, 10000, NULL); assert(hRead != INVALID_HANDLE_VALUE);

SECURITY_ATTRIBUTES inherit = { sizeof(SECURITY_ATTRIBUTES), NULL, TRUE }; HANDLE hWrite = CreateFileW(name, GENERIC_WRITE | SYNCHRONIZE, 0, &inherit, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); assert(hWrite != INVALID_HANDLE_VALUE);

STARTUPINFOW si = { sizeof(STARTUPINFOW) }; si.dwFlags = STARTF_USESTDHANDLES; si.hStdOutput = hWrite; PROCESS_INFORMATION pi; wchar_t cmdLine[] = L"cmd /c ver"; assert(FALSE != CreateProcessW(NULL, cmdLine, NULL, NULL, TRUE, CREATE_NO_WINDOW, NULL, NULL, &si, &pi));

CloseHandle(hWrite); CloseHandle(pi.hThread);

OVERLAPPED ovl = { 0 }; ovl.hEvent = CreateEvent(NULL, TRUE, TRUE, NULL); char buffer[2048]; DWORD len;

bool reading = false; while (WaitForSingleObject(pi.hProcess, 0) != WAIT_OBJECT_0) { if (!reading) { ReadFile(hRead, buffer, sizeof(buffer), NULL, &ovl); reading = true; } if (WaitForSingleObject(ovl.hEvent, 100) == WAIT_OBJECT_0) { reading = false; if (GetOverlappedResult(hRead, &ovl, &len, FALSE)) { std::cout << std::string(buffer, len); } else { DWORD error = GetLastError(); if (error != ERROR_BROKEN_PIPE) { std::cerr << "Error " << GetLastError() << std::endl; } break; } } } if (reading) { if (GetOverlappedResult(hRead, &ovl, &len, TRUE)) { std::cout << std::string(buffer, len); } } CloseHandle(pi.hProcess); CloseHandle(ovl.hEvent); CloseHandle(hRead);

So if you're not getting the same behaviour, then you've probably lost a handle somewhere that needed to be closed (perhaps the move bugs?). This code won't deadlock if the process finishes, although since it's faking asynchrony it will deadlock if it does a blocking wait for termination without finishing the read.

Perhaps the most important observation is that if you comment out the CloseHandle(hWrite), then it *will* deadlock.

Alright I'm starting to doubt my sanity. I'm quite certain I tested that, even before I've written the async_pipe class etc. and I tried to get some clear information out of the msdn about that. And I thought this was strange behaviour, but only for the named pipes. I have no clue what I did wrong, because I could reproduce what you said using asnyc_pipe just now. Nevertheless, I am very happy I've been wrong, thank you so much. That is the on part where I wasn't very confident about the library, I'll add the feature after the review, since that also needs a lot of testing. So the behaviour that will be implemented then is quite simply: if you use a pipe or pstream with a process the unused side will be closed (also on error) and if necessary (on posix) it'll set some flags. If you want to avoid the automatic closing for whatever reason, you can just duplicate the pipe: ipstream p; pipe p2 = p.pipe(); //duplicate child c("foo", std_out > p2); int i; p >> i; //going for the deadlock And that would of course also mean, that you'd have a close pipe after this: pipe p; child c1("foo", std_out>p); child c2("bar", std_in < p); It'll also be closed on error, so we have consistent behaviour, if some decides to do that: std::error_code ec; ipstream is; child c("invalid-file", std_out > is, ec); int i; is >> i; I hope that makes sense, if the library passes the review, I will fix that before it goes into boost.

On 1/11/2016 21:41, Niall Douglas wrote:

You also don't need to use completion ports like ASIO does for async i/o on Windows which are slightly complex to use. There is a much simpler solution called alertable i/o which AFIO v2 uses which has NT dispatch for you all pending async i/o callbacks every time you sleep a thread. Examine AFIO v2's source code for ideas if you like, you can throw together an async pipe i/o multiplexed reactor with very little effort in no time because NT has already implemented an "ASIO" for you, no ASIO needed.

Having written my own mostly-lock-free ASIO-lite based around completion ports, I can confirm that they're not at all complex to use. Meanwhile I shy away from alertable I/O because it's thread-affine and requires remembering to always perform alertable waits, both of which seem fragile to me. Still, the advantage of both methods existing is that you can use whatever you feel most comfortable with. :)

On 1 Nov 2016 at 23:57, Klemens Morgenstern wrote:

Am 01.11.2016 um 23:48 schrieb Gavin Lambert:

...

On 1/11/2016 21:41, Niall Douglas wrote:

...

You also don't need to use completion ports like ASIO does for async i/o on Windows which are slightly complex to use. There is a much simpler solution called alertable i/o which AFIO v2 uses which has NT dispatch for you all pending async i/o callbacks every time you sleep a thread. Examine AFIO v2's source code for ideas if you like, you can throw together an async pipe i/o multiplexed reactor with very little effort in no time because NT has already implemented an "ASIO" for you, no ASIO needed.

Having written my own mostly-lock-free ASIO-lite based around completion ports, I can confirm that they're not at all complex to use. Meanwhile I shy away from alertable I/O because it's thread-affine and requires remembering to always perform alertable waits, both of which seem fragile to me. Still, the advantage of both methods existing is that you can use whatever you feel most comfortable with. :)

I guess the right approach for boost.process would be, to add support for boost.afio when it comes out, as it does now for boost.asio. Elsewise you'd just have multiple libraries doing the same thing.

AFIO should land in the peer review queue in 2019. Outcome, upon which AFIO v2 is very heavily based (and probably would be the most contentious part of AFIO v2's API design), should land in the peer review queue early 2017 (I'm currently writing its tutorial at https://ned14.github.io/boost.outcome/). AFIO v2 was designed to understand child process handles and pipes, and its hierarchy of class inheritances allows easy extension to implement child process i/o support. Pull requests for that are welcome. BTW it is the lack of user supplied i/o backends in Process which was a big part of why I voted to reject it. The correct API design would allow the user to supply *any* implementation of child process i/o, and Process would not need to care what it is nor how it works. You would of course supply a default i/o backend probably based on popen() on POSIX or something equally simple, but the point is that you having to hard code support for ASIO or AFIO shows how Process has the wrong API design. My other big showstopper issue was you imposing on users your serialisation API i.e. all that iostreams and FILE * machinery. It's widely known that the committee would like a v2 iostreams to happen as soon as possible as the current iostreams is so very 1992. Quite a few people are experimenting with some very alternative designs right now, again if Process didn't need to be hardcoded to use some serialisation framework that would be a huge gain and in my mind the correct API design. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

On 2/11/2016 21:57, Niall Douglas wrote:

It's true a lot of people will be surprised AFIO v2 has a totally different async i/o engine design to ASIO. The easy answer is that POSIX AIO gives you no choice, but the fuller answer is that I had a small contract last year to give some expert consulting advice on a very, very high end storage system which was experiencing unhelpful random latency spikes. I benchmarked IOCP vs alertable vs synchronous, and the performance increased in exactly that order i.e. synchronous is easily the fastest, alertable not bad if you don't overload a given thread, IOCP awful.

Interesting. I had a serial-port-based application which used a thread-per-port model with synchronous I/O (technically still overlapped, but it used WaitCommEvent then read that many bytes, so reading was never really async). Latency spikes got quite bad as the number of ports rose. Switching it to an ASIO thread-agnostic reactor model helped a little but not much (mostly due to strand locking), but using "raw" IOCP worked wonders.

Am 28.10.2016 um 02:12 schrieb Tom Kent:
> On Thu, Oct 27, 2016 at 1:26 AM, Antony Polukhin 
> wrote:
>
>
>> Dear Boost community,
>>
>> The formal review of Klemens David Morgenstern's Process library
>> begins today, 27th October and ends on 5th November.
>>
>> We encourage your participation in this review. At a minimum, kindly
> state:
>> - Whether you believe the library should be accepted into Boost
>>    * Conditions for acceptance
> I believe this is borderline, with some serious quality issues, but should
> be accepted.
> With the following conditions:
> * Documentation is cleaned up substantially (see issues and pull request I
> submitted in github)
> * The documentation indicates that "There is no guarantee in which order
> the arguments will be applied!". This either needs to be fixed so that the
> order is always guaranteed or it needs to be greatly elaborated on as to
> when the order is kept. Process argument order is very often important.
It seems this statement is too ambiguous, meant are the arguments of the 
function, not the process. If you mix the property and overload style, 
the library will resort them in some way. I.e.:

bp::system("foo", "bar1", std_out > bp::null, "bar");

This will keep the arg-vector in order, i.e. {"bar1", "bar2"} but you 
have no guarantee if the std_out or the argv is applied first. This is 
only important for user extensions.
> * There needs to be a warning (or at least explanation of the implications
> of or resilience to) user-input going to command line calls with relation
> to shell injection vulnerabilities[1]. The python subprocess module has a
> good warning message we could probably borrow [2] if necessary.
Will be added to the shell reference.
> * Needs to specify setup (explain that this is a header-only library) and
> dependencies (the basic example needs boost::filesystem, even though it
> just uses strings?).
> * Need to fix introduction/tutorial examples (probably the library
> implementation) so they run on windows (and use the path to find exes).
That's not a good idea, because it incurs overhead (searching the path).
>
>
>> - Your knowledge of the problem domain
> Medium, from a user perspective. I've worked extensively with Python's
> similar Subprocess module.
>
>
>
>>
>> You are strongly encouraged to also provide additional information:
>> - What is your evaluation of the library's:
>>    * Design
> I like the design very much. The one big issue that I'm unsure about is
> that it is difficult to break out of a loop based on a pstream when the
> program exits. Maybe some EOF signal needed?
>
> I like the basic system/spawn/child design.
>
> Though I didn't dig too deep, the Sync/Async IO seems good.
>
> It seems weird that terminating a process doesn't terminate its children by
> default and you need to use a group. It seems like that should be an option
> of the child object (defaulted to on).
That's just the behaviour of the OS, which this library tries to mirror 
closely. Grouping is unecessary for many applications, that's why it's 
not the default.
>
> Environment is OK, is there a reason this_process isn't in the process
> namespace?
Well, for the same reason it's std::this_thread; 
boost::process::this_process would look weird, at least to me.
>
>>    * Implementation
>  From what I've seen running on windows, there are some prominent holes in
> the implementation. These seem to be bugs that could be fixed quickly.
>
>
>
>>    * Documentation
> Needs lots of help. Could also use more complex examples. I'm also left
> with the suspicion that there is more to the library than is covered by the
> docs. Some of the things mentioned in the tutorial seem to have other ways
> to use them, these should be called out in specific documentation for the
> items.
Well the reference is rather detailed, but if you have any common 
examples please let me know. It's not that easy to find a tool that 
everyone knows.
>
>
>>    * Tests
> I didn't actually look at the tests, but was disappointed to see that there
> was only 44% coverage.
Well gcov or lcov isn't happy with to deep template instanciations. The 
on_exec_setup is here is guaranteed to be executed, but for some reason 
that doesn't show: 
https://coveralls.io/builds/8547269/source?filename=%2Fhome%2Ftravis%2Fboost-local%2Fboost%2Fprocess%2Fdetail%2Fposix%2Fpipe_out.hpp

My local build is > 70%.
>
>>    * What was the experience? Any problems?
> msvc-12.0 - Visual Studio 2013 - Test suite failed.
> msvc-14.0 - Visual Studio 2015
>
> I ran into issues with some pretty basic examples.
>
>          boost::filesystem::path p = "python.exe";
>          std::system("python.exe --version"); // Works
>          bp::system("python.exe --version"); // Works
>          bp::system("python.exe", "--version"); // Fails
>          bp::system(p, "--version");  // Fails
>          bp::system("C:\\Python27-32\\python.exe", "--version"); // Works
>
> It seems that the library isn't correctly working with the path on windows,
> when the arguments are split from the executable.
It's not working at all with path, it depends on the OS. The cmd-style 
searches path, while he exe-args style does not. That's the weird 
platform dependent behaviour, I'm not elaborating on enough.
Boost.Process does not search the path automatically, it just uses the 
defaults of the OS. If you want to search the path, you have the 
search_path function for that, i.e. turn the first line into

boost::filesystem::path p = bp::search_path("python.exe");
>> - How much effort did you put into your evaluation of the review?
> A few hours.
>
> I hope these issues can get resolved and this can be included with boost!
> Tom
>
> [1] https://en.wikipedia.org/wiki/Shell_injection#Shell_injection
> [2]
> https://docs.python.org/3/library/subprocess.html#security-considerations
>
Thank you! I'll probably address most the issues this evening.

Am 30.10.2016 um 16:48 schrieb John Bytheway:

On 2016-10-28 09:04, Klemens Morgenstern wrote: <snip>

...

It's not working at all with path, it depends on the OS. The cmd-style searches path, while he exe-args style does not. That's the weird platform dependent behaviour, I'm not elaborating on enough. Boost.Process does not search the path automatically, it just uses the defaults of the OS. If you want to search the path, you have the search_path function for that, i.e. turn the first line into

boost::filesystem::path p = bp::search_path("python.exe"); So you are suggesting searching the path for the executable and then running it as a separate step? That sounds like it would introduce a race condition (if the executable is moved between being found and run) which could cause execution to fail where it would otherwise succeed (assuming there is another program with the same name later on the path).

That makes me uncomfortable. At the least, that warrants a warning in the docs, and I feel it ought to not be the recommended approach.

But maybe I'm misunderstanding; I haven't actually looked at your docs yet.

John You mean it's a race condition, because the OS would block moving of the file before launching? I guess that could happen, but that's true for all such operations, e.g. if during launch of foo.exe you move it's needed bar.dll. Or if you use boost::filesystem:

if (fs::exists(my_path)) fs::istream is(my_path); So yes if you write bp::system(bp::search_path("foo.exe")); you'd have a few cycles in between to move the file. However, I don't think that's the problem: you either have a command line, in which case the OS takes care of that or you have an absolute path. The search_path function helps you to obtain the latter, but does not lock it. I guess you could open a handle to the file to do that, but that would be very strange default behaviour.

On 10/31/2016 10:59 AM, Klemens Morgenstern wrote:

Am 31.10.2016 um 15:27 schrieb Edward Diener:

...

On 10/27/2016 2:26 AM, Antony Polukhin wrote:

...

Dear Boost community,

The formal review of Klemens David Morgenstern's Process library begins today, 27th October and ends on 5th November.

Process is a C++11 library to manage system processes. It can be used to: * create child processes * setup streams for child processes * communicate with child processes through streams (synchronously or asynchronously) * wait for processes to exit (synchronously or asynchronously) * terminate processes

Full documentation with examples and tutorial is available at http://klemens-morgenstern.github.io/process/index.html Stable source codes for review are available at https://github.com/klemens-morgenstern/boost-process/tree/boost_review Latest source codes available at https://github.com/klemens-morgenstern/boost-process

We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance - Your knowledge of the problem domain

You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems? - How much effort did you put into your evaluation of the review?

We await your feedback!

This is just an initial review of the process library.

I am looking at the documentation but I have not tried anything yet. I like the focus of the library. The tutorial is good but since some areas are pretty terse I was expecting a further explanation of some of the concepts in the library. But a further explanation does not exist. I am expected to understand how to use the library from the tutorial and reference. I think a discussion of using pipes, no matter how small it might be, for input and output should be part of the documentation. Similarly groups are mentioned in the tutorial without explaining their purpose at all. I guess a small section called "concepts" which explains the basics of what a pipe etc. is, and links to more detailed explanations, wouldn't hurt. It seems not everyone is hacking everything into google.

If you create your own library I do not think that Google is what you should be expecting people to use to understand your concepts and their functionality. That's why you write docs, so people understand what you are offering. If what you are offering is essentially the same syntax and functionality as something else which already exists and is explained adequately somewhere else then a link to that explanation should eb sufficient.

...

I found the notation for the std_out, std_err, and std_in to be exactly the opposite of what I would expect. I would think std_out and std_err would use a '<' notation and std_in would use a '>' notation. But using the pipes was fairly straightforward. I found the naming in the tutorial a bit strange, where a bp::opstream is called 'in' and a bp::ipstream is called 'out'.

That's the nature of the problem: output of the child is input for the father. Hence std_out is redirectin the stdout of the child into a ipstream, which is called out, because it's the output of the child proces. Strange, but actually makes sense.

I understand what you are saying. Naming variables in your tutorial is a your choice and has nothing to do with your library syntax. Still you might want to reconsider the std_out, std_err, std_in operator syntax since I think it runs contrary to what most would expect. But it is a really minor issue. I am much more intersted that you adequately explain all areas of functionality in your library, even briefly, rather than relying on people to discover them in the reference. For me personally the reference in documentation is just a syntactical document of functionality and a library author should not expect an end-user to dig into the reference to discover what a library has to offer.

...

In general I think the library is focused and very promnising. But I would still want something between the tutorial and the reference discussing all the main concepts of the library, so that the end-user does not have to dig in the reference to understand what areas of functionaity the library has to offer.

Am 31.10.2016 um 23:02 schrieb Gavin Lambert:

On 1/11/2016 03:27, Edward Diener wrote:

...

I found the notation for the std_out, std_err, and std_in to be exactly the opposite of what I would expect. I would think std_out and std_err would use a '<' notation and std_in would use a '>' notation. But using the pipes was fairly straightforward. I found the naming in the tutorial a bit strange, where a bp::opstream is called 'in' and a bp::ipstream is called 'out'.

bp::std_out > stdout bp::std_out > "output.txt" bp::std_in < "input.txt"

These seem the right way around to me, and reminiscent of how they're used in a shell; just with the extra keyword in front. Pretend the keyword is the process name instead.

Having said that, due to the presence of the keyword, perhaps assignment would be a better syntax, similar to bp::args? eg:

bp::std_out = stdout bp::std_out = "output.txt" bp::std_in = "input.txt"

This is allowed syntax, as is bp::std_out(stdout). I personally like the < and > syntax very much, but for those who don't there are alternatives.

Am 31.10.2016 um 23:41 schrieb Gavin Lambert:

On 1/11/2016 11:13, Klemens Morgenstern wrote:

...

...

bp::std_out = stdout bp::std_out = "output.txt" bp::std_in = "input.txt"

This is allowed syntax, as is bp::std_out(stdout). I personally like the < and > syntax very much, but for those who don't there are alternatives.

I don't see that mentioned anywhere in the docs, not even in the reference section. Did I miss it?

Also, while on the topic of missing things, the "Extensions" section mentions on_setup/on_error/on_success, but these don't appear to be listed in the Reference and it's unclear to me what purpose they serve (especially on_setup, which at first glance seems indistinguishable from just writing the code prior to calling the launch function).

It's hidden quite well, it's at the end of each section (i.e. file/pipe/close/null): http://klemens-morgenstern.github.io/process/boost/process/std_out.html Though I just realized, that there is no std_out("output.txt"), just the '='.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On 11/4/2016 6:28 PM, Vinnie Falco wrote:

On Fri, Nov 4, 2016 at 6:17 PM, Edward Diener wrote:

...

It always baffles me when a developer provides useful functionality in his library but does not bother to explain what that functionality entails. Is it laziness ? Is it tiredness ? Is it an inability to write a few coherent sentences in English because of lack of education or that English is not the developer's native language ? I just don't know.

I can help answer that question, at least for how it pertains to me. My inability/anxiety to write is directly proportional to the degrees of freedom for the text in question. The more constrained, the easier to write. The more freedom, the more difficult. I can easily write the first line of a Javadoc comment, explaining what the function does in a sentence. Similarly I can fill out the @param list with not much trouble. Adding extra paragraphs for the verbose description requires more thought but it is doable.

Sometimes though, the longer description requires above average work and that's where anxiety / writer's block starts to creep in. Do we talk about exceptions first? Undefined behavior? Where in the description would be a natural place to put an example? How big should the example be? Do we make the example compile completely stand-alone? Do we assume there's a "using namespace" in there?

Then we get to the part of the documentation which is unstructured and pure exposition. I'm talking about the part where there are no guide rails or Javadoc. Just an empty [section] in the .qbk file and oh - hey, now its time to write! I try to write Javadocs as I go because they are easy to update as needed. But I don't make a large investment in exposition until the library has reached a certain size and I have some confidence that things aren't going to change too much. With pure exposition there are even more choices to make. How to start the conversation? Do we start with an example? Explain the motivation? How detailed do we make this explanation? Do we split the advanced use cases to another section where they won't confuse people or do we keep everything related to individual functions and classes together regardless of complexity?

By the time I'm writing exposition, I've feel like I've already just finished a programming marathon and quite exhausted. Its tough to muster up the remaining creative energies to write. Writing code is in some ways easier than writing exposition. The compiler warns you or gives you an error if you made a mistake. And you can verify that the program is correct by writing tests and running it. Analagous systems exist for exposition: proof readers, friends and family, other programmers reviews. But they lack the certainty and responsiveness of the compiler.

I can't speak for others, but I am certainly not lazy. Perhaps "tiredness" is part of it. Good documentation requires considerable effort.

I agree that good documentation requres considerable effort, and I thank you for describing your own attempts at it.

On 11/4/2016 6:44 PM, Klemens Morgenstern wrote:

...

On 10/27/2016 2:26 AM, Antony Polukhin wrote:

...

Dear Boost community,

The formal review of Klemens David Morgenstern's Process library begins today, 27th October and ends on 5th November.

Process is a C++11 library to manage system processes. It can be used to: * create child processes * setup streams for child processes * communicate with child processes through streams (synchronously or asynchronously) * wait for processes to exit (synchronously or asynchronously) * terminate processes

Full documentation with examples and tutorial is available at http://klemens-morgenstern.github.io/process/index.html Stable source codes for review are available at https://github.com/klemens-morgenstern/boost-process/tree/boost_review Latest source codes available at https://github.com/klemens-morgenstern/boost-process

We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance - Your knowledge of the problem domain

You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems? - How much effort did you put into your evaluation of the review?

We await your feedback!

I vote to accept the Process library.

I highly suggest that the documentation be expanded beyond the tutorial to provide topics explaining all the major areas of the library. I don't feel that one-liner reference comments even come close to explain all the areas that the library covers. As an example of this I hunted in the reference to find that the library provides low-level access in its environment.hpp header to the process ID and the native process handle, but even here any explanation for what else is in the header, as well as its meaning, is almost non-existent. It always baffles me when a developer provides useful functionality in his library but does not bother to explain what that functionality entails. Is it laziness ? Is it tiredness ? Is it an inability to write a few coherent sentences in English because of lack of education or that English is not the developer's native language ? I just don't know. Ouch. Well I can't hide behind the language card after demonstrating

Am 04.11.2016 um 23:17 schrieb Edward Diener: that I'm rather able to write in english on the mailing list. The assumption is: if you use environment.hpp you already no what this is supposed to do

How does one know to use environment.hpp when 1) There is no previous documemtation that it exists 2) Even if one finds it the explanation of functionality in it is very terse. In other words you, the library developer, know that there is valuable functionality involved there, so why would you want to bury it with barely no explanation instead of explaining what you yourself have provided ? Or, better explained, why do the work of adding useful functionality to your library unless you are going to tell end-users of your library about it ? Library development is not a "find the treasure" game, it's an effort to create functionality which makes programming easier for a particular domain.

, since you read the OS documentation. The reason for that is that I was familiar with this stuff when I started to use boost.process 0.5. If you know that much, I think the documentation does suffice. But I would also be insincere if I claimed that writing documentation is my favorite thing in the world. But since this was now critized by many, I will add a chapter explaining all those concepts, i.e. pipes, environment, process groups and a bit of asio.

...

On the plus side the library provides basic useful functionality for what a process library should provide in a cross-platform way. I especially like the idea that the library is focused on this basic functionality and that the syntax for using that functionality is clear and easy to use. I also like the idea that the library relies on already established Boost functionality in asio for accomplishing more difficult tasks rather than trying to re-invent everything or trying to provide more all-purpose interfaces which nobody can understand how to use.

The library is obviously very useful whenever the task of creating and communicating with child processes needs to be done.

I tested the basic functionality with VC++14, update 3, on Windows. Everything worked as expected.

My effort was a few hours of reading documentation to understand what the library entailed and a few hours to test it out. I have a great deal of knowledge involving processes on Windows and only a small amount of knowledge involving processes on Linux.