patterncsharpMinor
Ensuring that events raised by system.diagnostics process class happen in the parent thread
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
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 (
If you want a simple solution though, yours is probably fine. A few things you might want to improve:
-
If
You should remove hardcoded "magic" strings either way.
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
whileloop to avoid arrow code. This can be done by usingbreak/continue/returnstatements 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 releaselockbefore callingXXX.
- 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.