Skip to content

Piped communication on Windows #6180

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Aug 23, 2021
Merged

Piped communication on Windows #6180

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ffacdea
Expose quote_windows_arg
TGWDB Aug 11, 2021
20c51db
Add in Windows version of piped_process functions
TGWDB Aug 11, 2021
fab21fe
Add unit tests for Windows piped process IPC
TGWDB Aug 11, 2021
c90b2fe
Fix missing copy operations (delete them)
TGWDB Aug 16, 2021
803de49
Fix unit tests to use std::chrono not Windows.h
TGWDB Aug 16, 2021
dfd28af
Fix destructor scope to fix termination test
TGWDB Aug 16, 2021
3f9fe4a
Fix include scoping and use unique_ptr
TGWDB Aug 16, 2021
c62310f
Fix to use system_exceptiont
TGWDB Aug 16, 2021
8510283
Split out helper function for Windows string construction
TGWDB Aug 17, 2021
f60acac
Reduce unecessary states for piped process statet
TGWDB Aug 17, 2021
cec11e1
Pass commandvec by reference and small fixes
TGWDB Aug 17, 2021
f7e4440
Update comment on timeout in windows
TGWDB Aug 23, 2021
e530e4a
Tidy up unit test code
TGWDB Aug 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions src/util/piped_process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,19 @@

# define BUFSIZE 2048

#ifdef _WIN32
std::wstring process_windows_args(const std::vector<std::string> commandvec)
{
std::wstring result = widen(commandvec[0]);
for(int i = 1; i < commandvec.size(); i++)
{
result.append(L" ");
result.append(quote_windows_arg(widen(commandvec[i])));
}
return result;
}
#endif

piped_processt::piped_processt(const std::vector<std::string> commandvec)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognise this was here before, but any insight as to why this isn't being passed as a reference? The const doesn't seem terribly useful at present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No insight, but fixed now.

{
// Default state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to split up this mega-function a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split out a small helper function, but the function is pretty straightforward, just many lines due to many arguments to function calls to create Windows processes/pipes (also lots of commends/explanation).

Expand Down Expand Up @@ -193,13 +206,7 @@ piped_processt::piped_processt(const std::vector<std::string> commandvec)
start_info.hStdOutput = child_std_OUT_Wr;
start_info.hStdInput = child_std_IN_Rd;
start_info.dwFlags |= STARTF_USESTDHANDLES;
// Unpack the command into a single string for Windows API
std::wstring cmdline = widen(commandvec[0]);
for(int i = 1; i < commandvec.size(); i++)
{
cmdline.append(L" ");
cmdline.append(quote_windows_arg(widen(commandvec[i])));
}
const std::wstring cmdline = process_windows_args(commandvec);
// Note that we do NOT free this since it becomes part of the child
// and causes heap corruption in Windows if we free!
const BOOL success = CreateProcessW(
Expand Down