4

Quick preface of what I'm trying to do. I want to start a process and start up two threads to monitor the stderr and stdin. Each thread chews off bits of the stream and then fires it out to a NetworkStream. If there is an error in either thread, both threads need to die immediately.

Each of these processes with stdout and stdin monitoring threads are spun off by a main server process. The reason this becomes tricky is because there can easily be 40 or 50 of these processes at any given time. Only during morning restart bursts are there ever more than 50 connections, but it really needs to be able to handle 100 or more. I test with 100 simultaneous connections.

try
{
    StreamReader reader = this.myProcess.StandardOutput;

    char[] buffer = new char[4096];
    byte[] data;
    int read;

    while (reader.Peek() > -1 ) // This can block before stream is streamed to
    {
        read = reader.Read(buffer, 0, 4096);
        data = Server.ClientEncoding.GetBytes(buffer, 0, read);
        this.clientStream.Write(data, 0, data.Length); //ClientStream is a NetworkStream
    }
}
catch (Exception err)
{
        Utilities.ConsoleOut(string.Format("StdOut err for client {0} -- {1}", this.clientID, err));
        this.ShutdownClient(true);
}

This code block is run in one Thread which is right now not Background. There is a similar thread for the StandardError stream. I am using this method instead of listening to OutputDataReceived and ErrorDataReceived because there was an issue in Mono that caused these events to not always fire properly and even though it appears to be fixed now I like that this method ensures I'm reading and writing everything sequentially.

ShutdownClient with True simply tries to kill both threads. Unfortunately the only way I have found to make this work is to use an interrupt on the stdErrThread and stdOutThread objects. Ideally peek would not block and I could just use a manual reset event to keep checking for new data on stdOut or stdIn and then just die when the event is flipped.

I doubt this is the best way to do it. Is there a way to execute this without using an Interrupt?

I'd like to change, because I just saw in my logs that I missed a ThreadInterruptException thrown inside Utlities.ConsoleOut. This just does a System.Console.Write if a static variable is true, but I guess this blocks somewhere.

Edits:

These threads are part of a parent Thread that is launched en masse by a server upon a request. Therefore I cannot set the StdOut and StdErr threads to background and kill the application. I could kill the parent thread from the main server, but this again would get sticky with Peek blocking.

Added info about this being a server.

Also I'm starting to realize a better Queuing method for queries might be the ultimate solution.

Mark
  • 867
  • 8
  • 12

4 Answers4

3

I can tell this whole mess stems from the fact that Peek blocks. You're really trying to fix something that is fundamentally broken in the framework and that is never easy (i.e. not a dirty hack). Personally, I would fix the root of the problem, which is the blocking Peek. Mono would've followed Microsoft's implementation and thus ends up with the same problem.

While I know exactly how to fix the problem should I be allowed to change the framework source code, the workaround is lengthy and time consuming.

But here goes.

Essentially, what Microsoft needs to do is change Process.StartWithCreateProcess such that standardOutput and standardError are both assigned a specialised type of StreamReader (e.g. PipeStreamReader).

In this PipeStreamReader, they need to override both ReadBuffer overloads (i.e. need to change both overloads to virtual in StreamReader first) such that prior to a read, PeekNamedPipe is called to do the actual peek. As it is at the moment, FileStream.Read() (called by Peek()) will block on pipe reads when no data is available for read. While a FileStream.Read() with 0 bytes works well on files, it doesn't work all that well on pipes. In fact, the .NET team missed an important part of the pipe documentation - PeekNamedPipe WinAPI.

The PeekNamedPipe function is similar to the ReadFile function with the following exceptions:

...

The function always returns immediately in a single-threaded application, even if there is no data in the pipe. The wait mode of a named pipe handle (blocking or nonblocking) has no effect on the function.

The best thing at this moment without this issue solved in the framework would be to roll out your own Process class (a thin wrapper around WinAPI would suffice).

Community
  • 1
  • 1
Zach Saw
  • 4,308
  • 3
  • 33
  • 49
  • .NET Team: If you're reading this, head to my blog post for the fix - http://blog.zachsaw.com/2011/07/streamreaderpeek-can-block-another-net.html – Zach Saw Oct 05 '11 at 04:57
  • maybe you should propose this fix to .NET Core? – knocte Jan 06 '17 at 09:37
0

You're building a server. You want to avoid blocking. The obvious solution is to use the asynchronous APIs:

var myProcess = Process.GetCurrentProcess();
StreamReader reader = myProcess.StandardOutput;

char[] buffer = new char[4096];
byte[] data;
int read;

while (!myProcess.HasExited)
{
    read = await reader.ReadAsync(buffer, 0, 4096);
    data = Server.ClientEncoding.GetBytes(buffer, 0, read);

    await this.clientStream.WriteAsync(data, 0, data.Length);
}

No need to waste threads doing I/O work :)

Luaan
  • 62,244
  • 7
  • 97
  • 116
  • why 4096 and not other number? – knocte Jan 06 '17 at 09:40
  • @knocte 4096 is what the OP used in his example. There are some benefits to some numbers, but that's quite a broad topic. For example, buffers of powers of two may be much friendlier to heap fragmentation, which is actually an issue when doing I/O in .NET - the buffers are pinned, so the heap can't be compacted. This is especially useful when pre-allocating buffers, since you can make sure the buffers waste no more than 50% of required memory (until optimal buffers run out, of course). But for the most part, it's just tradition - unless you know a better buffer size, powers work well. – Luaan Jan 06 '17 at 10:26
0

Why dont you just set both Threads to be backround and then kill the app? It would cause an immediate closing of both threads.

alex
  • 1,228
  • 1
  • 16
  • 38
  • I'll add this as an edit to the original question so others can easily see. But, this is a server that is spawning sometimes hundreds of these Processes with the two Stream threads. The whole application cannot die. I could tell the main server to kill this child thread, but again that gets sticky because of Peek blocking. – Mark Dec 29 '10 at 21:45
-1

Get rid of peek and use the method below to read from the process output streams. ReadLine() returns null when the process ends. To join this thread with your calling thread either wait for the process to end or kill the process yourself. ShutdownClient() should just Kill() the process which will cause the other thread reading the StdOut or StdErr to also exit.

    private void ReadToEnd()
    {
        string nextLine;
        while ((nextLine = stream.ReadLine()) != null)
        {
             output.WriteLine(nextLine);
        }
    }
Kevin
  • 2,281
  • 1
  • 14
  • 16
  • This is nice in theory and allows me to avoid Peek, but ReadLine still blocks. Unfortunately this means that both threads need to be monitored and resources cannot be released until both threads have stopped. Even with only 30 processes this kill operation can take 200 ms or more. Also I cannot close the NetworkStream connection until both threads have died in case the ReadLine returns, so that a Write does not occur. Ultimately throwing and catching a very rare ThreadInterrupedException is less to deal with than all the NullExceptions and SocketExceptions that can occur. – Mark Dec 30 '10 at 15:35
  • 1
    I would not write to the NetworkStream in the ReadToEnd() method above but to a queue. So after a line is read it gets put onto the queue either directly or inside another object which indicates if its from StdOut or StdErr (tweak this depending on what information you need). Then I would have another thread reading from the queue and writing to the NetworkStream. Obviously everything needs to be thread safe with locks etc. You can use a Waithandle so that as soon as one of the threads adds to the queue it does a WaitHandle.Set() signalling the consumer thread to go ahead and write to Network. – Kevin Dec 30 '10 at 17:21
  • Ah, that's actually a pretty good idea. I never thought of doing a producer/consumer type thing. This is definitely a lot safer than what I'm doing. I just got hung up on having the readers doing the writing as well. But this still doesn't handle when ReadLine blocks due to no data. Unfortunately .NET has not provided us with a nonblocking Read method. If no data is available any of the Read methods or Peek will block and I can only unblock it with an Interrupt or sometimes it will throw a null exception when the reader. There's no point in trading one exception for another. – Mark Dec 31 '10 at 15:52
  • You cannot wait for the process to end or Kill the process? Either of those will cause ReadLine() to return null and for the thread to end. – Kevin Jan 03 '11 at 13:55