[PATCH] Add non-blocking Wait() for launched processes

[Siraj, Tareq A]

See inline comments. Thanks.

-- 
Tareq A. Siraj

Software Developer
Intel of Canada Inc.




On 2013-09-26 1:32 PM, "Rafael Espíndola" <rafael.espindola at gmail.com>
wrote:

>On 25 September 2013 21:10, Tareq A. Siraj <tareq.a.siraj at intel.com>
>wrote:
>>
>>   @rafael: We need this to implement process based parallelism in the
>>clang-modernizer project. We are in the process of implementing this
>>feature into the modernizer, so expect to see it being used soon. See
>>https://cpp11-migrate.atlassian.net/browse/CM-107 for details.
>
>Cool. That is a good reason to add the feature back.
>
>>   Let me know if you have more comments. Thanks.
>>
>>
>> ================
>> Comment at: include/llvm/Support/Program.h:43
>> @@ +42,3 @@
>> +  // to pass around the process handle and the last object needs to
>>free it.
>> +  friend class llvm::RefCountedBase<ProcessInfo>;
>> +
>> ----------------
>> Rafael Ávila de Espíndola wrote:
>>> How is this different from other OS resources? A file must be closed
>>>for example, but having all files be refcounted is probably not a good
>>>design.
>>>
>> This is needed for the Windows implementation. CreateProcess() gives
>>you a process id and a handle to the process and someone needs to
>>release the handle. When we make copies of the ProcessInfo objects,
>>question becomes who calls CloseHandle(). Alternatively we can close the
>>handle and not store the handle but in that case we will need to call
>>OpenProcess() every time we call Wait (non-blocking).
>>
>> Let me know if you have any better implementation in mind.
>
>Can't we call CloseHandle in Wait and document that the user of
>ExecuteNoWait must call Wait?

That might work. For non-blocking Wait() we return a dummy ProcessInfo and
keep the handle open and for every other case close it. I'll give this a
try and update the docs to say either call wait or close the handle
manually if you don't need it (should be safe for running processes
according to the docs of CloseHandle()).

>
>> ================
>> Comment at: include/llvm/Support/Program.h:67
>> @@ +66,3 @@
>> +  /// The path to the executable this process is running.
>> +  llvm::SmallString<256> Program;
>> +
>> ----------------
>> Rafael Ávila de Espíndola wrote:
>>> This is just for the sys::fs::exists check in Wait, right? Does any
>>>code depend on it? If not, just delete it. In the unlikely case that we
>>>do depend on it, can you move the check to Execute and avoid having
>>>this member variable?
>> Yes, its the sys::fs:exists() check that needs it. We need to store it
>>somewhere because Wait() on Linux/posix_spawn implementation returns 127
>>for any kind of error (including file-not-found. See comment on
>>lib/Support/Unix/Program.inc:396 in this patch).
>>
>> Since Wait() is a public API function now, we need to store it
>>somewhere. Alternatively we can ask the user to pass it when calling
>>Wait() but that will be annoying for cases where you pass a
>>vector<ProcessInfo> to some function and you'll need to pass
>>vector<pair<ProcessInfo,string>>.
>
>But the only use it has in wait is to provide a better error message
>when the file doesn't exist. Since that is a race anyway, my
>suggestion is to do the check in Execute, before calling posix_spaw.
>That way you don't need to store it.

Good point. I'll update the code.

>
>>
>> ================
>> Comment at: include/llvm/Support/Program.h:157
>> @@ +156,3 @@
>> +      ///< until child has terminated.
>> +      std::string *ErrMsg = 0 ///< If non-zero, provides a pointer to
>>a string
>> +      ///< instance in which error messages will be returned. If the
>>string
>> ----------------
>> Rafael Ávila de Espíndola wrote:
>>> This is the old style of error handling. The new one would be for this
>>>function to return an error_code.
>>>
>> Its consistent with the current interface of Execute functions. I'll
>>suggest we improve the interface for all three in a separate patch.
>
>OK