HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Ensuring that events raised by system.diagnostics process class happen in the parent thread

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
raisedtheeventsdiagnosticsprocesssystemparenthappenthatthread

Problem

I'm quite new to threading primitives in C# and was hoping you might be able to suggest improvements to this. I need to ensure that the XXX call below happens within the calling thread (XXX is a foreign call into a thread-unsafe library), so I used a queue here. It seems a bit like there should be a better primitive for this. Maybe delegates are applicable somehow? I don't understand delegates.

I also have to wonder if I've gotten this whole scheme right in the first place! Maybe there's a deadlock I'm not seeing. Threading is so tricky.

As an additional restriction, it's very important that this works on .NET 3.5.

```
public void RunProc(AutoResetEvent killSubProc)
{
using (Process process = new Process())
{
var timeout = 8000;
var channel = new Queue {};

process.StartInfo.FileName = "blah.exe";
process.StartInfo.Arguments = @"stuff";

process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardOutput = true;
process.StartInfo.RedirectStandardError = true;
process.EnableRaisingEvents = true;

using (AutoResetEvent channelWaitHandle = new AutoResetEvent(false))
{
process.OutputDataReceived += (sender, e) => {
if (e.Data != null)
{
lock (channel) { channel.Enqueue("STDOUT"); channel.Enqueue(e.Data); }
channelWaitHandle.Set();
}
};
process.ErrorDataReceived += (sender, e) =>
{
if (e.Data != null)
{
lock (channel) { channel.Enqueue("STDERR"); channel.Enqueue(e.Data); }
channelWaitHandle.Set();
}
};
process.Exited += (sender, e) =>
{
lock (channel) { channel.Enqueue("EXI

Solution

If you want a pretty solution, you would have to separate your data source (process in your case) from data processor (your while loop), hide implementations behind interfaces and then wire those interfaces together via events or aggregation. This is actually a pretty common problem called "Producer-Consumer", you should be able to find multiple decent implementations on the net.

If you want a simple solution though, yours is probably fine. A few things you might want to improve:

  • Reduce nesting in your while loop to avoid arrow code. This can be done by using break/continue/return statements or by extracting logic to separate method or entity.



  • Depending on how often you receive messages and on how much time XXXtakes, you might want to release lock before calling XXX.



  • You might want to wait for process to exit before returning form your method. This will ensure, that everything went smoothly, and there is no process left hanging somewhere due to some error in your code.



-
If XXX signature is not set in stone, you should probably replace strings with complex object:

class Message
{
   //use enum instead of hardcoded strings, such as "STDERR"
   public MessageType Type { get; set; }
   //actual data from process
   public string Data { get; set; }
}


You should remove hardcoded "magic" strings either way.

Code Snippets

class Message
{
   //use enum instead of hardcoded strings, such as "STDERR"
   public MessageType Type { get; set; }
   //actual data from process
   public string Data { get; set; }
}

Context

StackExchange Code Review Q#112846, answer score: 4

Revisions (0)

No revisions yet.