[skip ci] ween part 2: electric boogaloo

Reporter: time-killer-games  |  Status: open  |  Last Modified: August 25, 2020, 05:49:25 PM

TODO: how tf to make this async
time-killer-games  
@RobertBColton it appears the only way to make this async is to use the FILE_FLAG_OVERLAPPED flag via CreateFile, set ReadFile's lpNumberOfBytesRead parameter to NULL, and use the last parameter of ReadFile. But we aren't using CreateFile with this code, so not sure how I am supposed to do it? https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile

Also I removed one of our break's. I don't know if I was supposed to do that or not...

RobertBColton  

I'm not sure, let me do some more research.

I think it might be why Microsoft was using CreateFile in their example, it's possible to connect your pipe to a file.
https://stackoverflow.com/q/51127722

Yes, you were correct to remove the break.

RobertBColton  

It seems as though it's just not possible with CreatePipe so you need to use CreateNamedPipe for the one we want to read. If it's simpler, you could try leaving the other pipes use CreatePipe since we are only interested in asynchronous behavior for the output pipe.
RobertBColton  

@time-killer-games In other words, the anonymous pipe you were using before? It was actually still a named pipe but just used the default name. Pipes are almost always backed by memory mapped files for interprocess communication. So even though you didn't realize it, because it was all abstracted away from you before, you were still in fact creating a file for the pipe!

A good example of this is how when you are using the Linux/Unix equivalents, you are working with file handles, even if you don't directly create a file. The pipe itself is a special file.

time-killer-games  

Sorry, I got sidetracked working on other things and then went to bed. I'm reading over your responses now.
time-killer-games  

@RobertBColton what do you suggest I do for this pr? I'd strongly prefer to just revert this back to fully working code, if we can't manage to find an approach you like better any time soon.
RobertBColton  

What are you talking about? This pull request was fine, all you needed to do was figure out the CreatedNamedPipe API and use the async version of ReadFile. I was waiting on you to do that.
time-killer-games  

I have no idea how to do anyone this, I can read the docs on those functions, but idk how that will tell me to do exactly what you are after. I'll try it though.
RobertBColton  

I can attempt it later if you want but I'm focusing on platforms atm.

You see the thing is, even if you put back your regular message pump you had from before, I think you are still going to hit the 4096 character limit, because you need to read while the process is running. So reverting your code is absolutely pointless here, it will just make it worse. You're better off to AT THE VERY LEAST keep the part we already fixed and just add back your message loop.

You have no other way to get around this, except the thread like I mentioned, which is even more painful than just figuring out CreateNamedPipe.

time-killer-games  

I've exceeded that many characters significantly though. I'll take your word for it. You understand that stuff a lot better than my crappy guess work and copy pasta.
codecov[bot]  

Codecov Report

Merging #2031 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2031   +/-   ##
=======================================
  Coverage   30.93%   30.93%           
=======================================
  Files         197      197           
  Lines       19102    19102           
=======================================
  Hits         5910     5910           
  Misses      13192    13192           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dab5494...9931066. Read the comment docs.

time-killer-games  

@RobertBColton if your idea doesn't do what we hope it will, any chance we could fall back on the logic from my original code, which is mostly based on this?: https://github.com/time-killer-games/libprocinfo/blob/dfb267bdea8230e6724ecdd8db4c516aa5465569/ProcInfo/Win32/procinfo.cpp#L146

...except returning the output, instead of the dead pid, and not removing trailing new lines.

RobertBColton  

I said, it's not going to work, it should hypothetically have the exact same issue as it originally did. I asked you to try it though out of curiosity.
time-killer-games  

@RobertBColton But it does work. I have been using it in its current state for about a month now, and it doesn't hang; I have successfully exceeded well over 4096 characters, in fact, I've done well over twice that amount. You can't just throw out the blanket statement "It's not going to work", without testing it, when it actually does work, if you cared enough to try it yourself.

I told you this already, and I said "I'll take your word for it", which meant i was trusting what you said in preference over reality, because I didn't feel like arguing with you over it.

RobertBColton  

Different kind of limit now Sam, just because you haven't hit the Windows buffer limit doesn't mean it's not there.
time-killer-games  

I gotcha, I mean to be technical I could verify that by adding characters to the printing until there is a stack overflow.
RobertBColton  

Here's another thing Sam, let me put it to you this way. Perhaps the idea behind execute_shell_for_output is not the best choice of API design. Think about it, even if not hitting the Window's buffer limit, they will hit the buffer limit for string. Here's what I wanted to propose to you, perhaps the user-facing API could be changed to just be like Windows and give the GM user a piece of the buffer at a time. Then they could get theoretically unlimited output from the process.
RobertBColton  

Rusky disagrees with me there though, but we could offer both APIs.
time-killer-games  

Would it hang if he hit the max buffer for string, or would it truncate? It might even crash. I'm not sure. I'd like to find out a the maximum buffer size possible and once it hits that just have it truncate, because no one will need the entire output if it will be that long. The point of this api is to get printed output from small command line apps, which usually don't really have but so much to print from a single command line anyway.

I'm testing with this right now:

#include <cstdio>
#include <string>

using std::string;

int main(int argc, char **argv) {
  if (argc == 2) {
    for (unsigned long long i = 0; i < stoull(string(argv[1]), nullptr, 10); i++) {
      printf("%llu", i);
      printf("%s", "\r\n");
    }
  }
  return 0;
}

and there comes a certain point where i don't know whether it is hanging or just taking a long ass time to process the request.

RobertBColton  

It will do whatever happens if you make a for loop and append to a string until it runs out of memory. Take this function completely out of the equation, that's what happens. It's not a huge deal, the API is fine for typical use honestly.
time-killer-games  

@RobertBColton we have 3 options here.

  1. We use my code that works better than what we have in master
  2. We use CreateNamedPipe like you wanted (which is fine if you want to write it)
  3. We close this pull request and accept the function in its currently unusable state

I'm willing to wait however long for you to set aside time to write it if we do 2).

time-killer-games  

@RobertBColton The code now works with VERY FUCKING
time-killer-games  

as in, very fucking long ass printing.
time-killer-games  

@RobertBColton I made it return printing that prints counting from 0 to 18446744073709551615, which each line printed increasing the count by 1, and I left it running for a whole 5 minutes and it didn't hang or crash, it just kept on printing more and more!
RobertBColton  

Hold up Sam, I think you're still not getting it.

option a: execute the program to completion, then read all the data out of the pipe
option b: execute the program, and as it runs read chunks of data out of the pipe

with option a, you will ALWAYS use AT LEAST 2x the amount of memory you need
option b only crashes if the string itself is so big it doesn't fit in enigma's address space

time-killer-games  

@RobertBColton I'm using option b now thanks to your suggestion to try threading it fixed all the former issues we've had with this. Any other objections you have left before this is ready to be merged? P.S. using enigma::handleEvents instead of where I am using GetMessage and PeekMessage doesn't work. So I had to get rid of the enigma::handleEvents and use the current code instead to get it working.
time-killer-games  

@RobertBColton @JoshDreamland Appveyor took a shit. Please restart the crapped jobs when you can. Thanks!
time-killer-games  

@RobertBColton my hymen popped fuck joo bitch !!!!
time-killer-games  

@RobertBColton please merge kthx
time-killer-games  

@JoshDreamland pls merge kthx
Please sign in to post comments, or you can view this issue on GitHub.