Motivation: In Windows, ::system() causes a terminal console window to pop up. This window grabs the focus away from the user, an irritating interruption to workflow.
Solution: Attempt to use CreateProcess() first. If this doesn't work for any reason, fall back to the previous solution and call ::system().
692 | + info.cb = sizeof(STARTUPINFO); 693 | + info.dwFlags = STARTF_USESHOWWINDOW; 694 | + info.wShowWindow = SW_HIDE; 695 | + PROCESS_INFORMATION processInfo; 696 | + ZeroMemory(&processInfo, sizeof(PROCESS_INFORMATION)); 697 | + if (CreateProcess(NULL, &strCommand[0], NULL, NULL, false, 0, NULL, NULL, &info, &processInfo))
please use strCommand.c_str() (if you need zero-terminated) or strCommand.data() (if no zero terminator is necessary) i.s.o &strCommand[0]
703 | + CloseHandle(processInfo.hThread); 704 | + CloseHandle(processInfo.hProcess); 705 | + nErr = (int)nExitCode; // Had to pass a ULONG to GetExitCodeProcess(). 706 | + } 707 | + else 708 | + {
Are you sure that this fallback is needed? I'm not sure what the implementation of mingw's system() does internally, but I'd assume it also makes use of CreateProcess or another win32 call.
Laanwj thanks for the feedback. Not quite sure why I did that &strCommand[0], I'll fix and reroll today. Regarding the second item, I believe system() handles more types of files, e.g. .bat files and document opening. I could be wrong about this. I will do some experimenting with both calls under MinGW and also possibly reroll that today.
Perhaps you can call ShellExecute, instead of CreateProcess?
Yes, at least bat files would need to be supported. Document opening isn't that terribly important here, if one really wants to do that they could provide the program to launch it with in the command line.
Looks like a good start. Needs more testing.
Okay finally had some time to research this a little better. Thanks everyone here for the suggestions.
I am not quite sure how to proceed, so here is what I have discovered, perhaps everyone here can make suggestions about the best route forward.
Here's the gist of the program I used for testing. Edit: oh, right. don't forget to build using mingw with -mwindows flag.
Edit2: Whoops my bad, Sleep takes milliseconds arg not seconds. Gist and table below updated, self exe is not always async. Thought that seemed funky. And really the "s" of the whole CreateProcess() column is because my code waits. We could change that behavior and be always async there.
| CreateProcess() | ShellExecute() | ::system() | |
|---|---|---|---|
| .bat script | n s | n a | p s |
| self exe | n s | n a | p s |
| pythonw.exe | n s | n a | p s |
| python.exe | n s | n a | p s |
<sup>Key: p = popup, n = no popup, s = synchronous, a = asynchronous</sup>
Each method has own strengths and weaknesses.
Pros
Cons
Pros
Cons
Pros
Cons
Thoughts and opinions? Thanks again for the feedback!
Seems to me that CreateProcess comes out best. Especially if it can work without fallback to ::system.
Why this affects the non "W" version I have no idea. Is the fix to copy the string? Or to just cast c_str() to (char*)? Unsure.
Just make a copy. Strings are not allowed to be mutated in-place in C++, but you could pass a begin_ptr(x) of a std::vector instead.
Ok updated. Nice call on begin_ptr() laanwj that makes it nice and clean. Since we removed the fallback wasn't quite sure what to do with nErr in that case, so just set it to -1 if CreateProcess() failed.
Does the walletnotify hook need to be stress tested here? Sometimes there are quite a few created simultaneously.
@kanzure well it's anecdotal but I did just test 10 days of blocknotify with that last commit. Worked great, barely a blip on the CPU radar (admittedly fast 4 core modern machine). My test script this time is just a .bat piping the block hash to a file. It added 145 blocks to the output file really quickly and was not noticeably different from running it without the script. Would walletnotify be appreciably different from that? What's a good way to test it I wonder? Guess I could trash my blockchain and let it reload with a walletnotify script. Suggestions on what to put in it?
Edit: oh hum I probably don't have enough tx in my testnet wallet to really "stress" test anything. Other ideas for approaches to this?
Edit2: watchonly addresses I guess, could just add tons of them. Is this worthwhile?
Edit3: Well I'm not really sure how to measure the results of this, so I'm leaning towards not testing walletnotify explicitly for performance. Intuition tells me this is a more efficient solution than ::system() just for the raw fact that Windows doesn't have to setup/teardown a window for each launch.
I'm confused why Travis failed here. Is that something in my fork? Did I screw up the merge from bitcoin/master?
Thanks a lot to jonasschnelli who helped me a ton in IRC figuring out how to work with github (this is my first pull request!)
@laanwj It seems that the fallback was necessary for the case of calling a terminal command like the "echo" in this test case (which is what fails w commit 01087f7 above).
I can imagine one other solution to this which I just tested with my gist (updated) locally. We can prepend C:\Windows\System32\cmd.exe /C to the user command and force it to execute in a shell. Of course, the code would need to be better than just hard coding C:\Windows.
tl;dr: The two choices I can think of is either use the fallback solution or call an interpreter by hand.
Other ideas?
Edit: oh I'd just like to add, using the interpreter actually works very nicely. No popup and it pipes to file as expected. My vote on how to implement this is heading in that direction.
Edit2: just updated the gist to use GetSystemDirectory(). Could work. Edit3: gist makes a vector. Edit4: it now does that in a nicer way using a stringstream. Edit5: Travis seems okay with this solution :+1:
Still no opinions? Well it's easy for me to change this code to go either route, fallback or call cmd.exe. I would really like to have some solution for this even if it still pops up the window in some cases (the fallback solution).
@dfletcher It's IMO not necessary to support providing a terminal command directly. As I said, what needs to be supported are .exe's and .bat's. The rest can be done from there.
As said, if remotely possible there should only be one path and no chain of fallbacks. If it fails, it fails. And simple code is preferable to complex code.
@laanwj there is a test in the system which is testing using echo. This fails if we do not provide a fallback or use the cmd.exe interpreter directly. See the Travis failure above + two successes afterwards, failure uses no fallback or interpreter, first success is just putting the fallback back, second success is with an interpreter.
So that leaves (1) Use fallback. (2) Use an interpreter. (3) Remove this test that uses "echo" which is not an exe it's an interpreter command.
You seem to be suggesting #3 which doesn't sound good to me. Changing the test conditions because of a difference in the API calls we're making seems backwards to me.
-alertnotify is a bitcoind, not Bitcoin-Qt, feature-- I wouldn't expect people running Bitcoin-Qt to run with -alertnotify, they're notified of alerts through the GUI.
So I think I'm NACK on this pull request in general for solving a problem that I don't think needs solving (command window popping up if you -alertnotify with Bitcoin-Qt).
@gavinandresen It solves -blocknotify and -walletnotify popping up a window also. -alertnotify is incidental.
Edit hum maybe I'm not being super clear about this. All three of those functions use the same utility function runCommand(). The fact that that test is checking -alertnotify is not exactly important. What is important is that I believe whoever wrote that test wants to make sure that the command processor can handle a command like "echo %s >> poo.txt" which is a perfectly reasonable implementation of any of the notifies, stuff it in a file and read it back later in some other process.
712 | + char system32[256]; 713 | + if (GetSystemDirectory(system32, 255)) { 714 | + std::stringstream ssCommand; 715 | + ssCommand << system32 << "\\cmd.exe /C " << strCommand; 716 | + std::string strTempCmd = ssCommand.str(); 717 | + std::vector<char> vecCommand(strTempCmd.begin(), strTempCmd.end() + 1);
It is not allowed to use end()+1: end() is the end of the data. There is guarantee that there is anything after it, let alone a \0 byte. Only c_str guarantees a zero-terminated string.
This is becoming quite ugly, but I guess what you could do is (this gets rid of the stringstream too)
std::string strTempCmd = std::string(system32) + "\\cmd.exe /C " + strCommand;
const char *sTempCmd = ssCommand.c_str();
std::vector<char> vecCommand(sTempCmd, sTempCmd + strlen(sTempCmd) + 1);
OTOH I don't see anywhere in the CreateProcess documentation that it will actually try to change the passed-in string. So this should suffice:
std::string strTempCmd = std::string(system32) + "\\cmd.exe /C " + strCommand;
if (CreateProcess(NULL, const_cast<char*>(strTempCmd.c_str()), NULL, NULL, false, 0, NULL, NULL, &info, &processInfo))
713 | + if (GetSystemDirectory(system32, 255)) { 714 | + std::stringstream ssCommand; 715 | + ssCommand << system32 << "\\cmd.exe /C " << strCommand; 716 | + std::string strTempCmd = ssCommand.str(); 717 | + std::vector<char> vecCommand(strTempCmd.begin(), strTempCmd.end() + 1); 718 | + if (CreateProcess(NULL, begin_ptr(vecCommand), NULL, NULL, false, 0, NULL, NULL, &info, &processInfo))
Code style: we have { on the same line as the if/else.
I do tend to agree with @gavinandresen that it'not s terribly useful to use -*notify with the GUI. For the daemon bitcoind.exe this is not an issue as it has its own shell window so should never pop up a new one?
Closing due to inactivity. There are still some comments open. Let me know when you start working on this again, and I'll reopen.