I have a windows forms app that launches up to X number of console processes concurrently. I'm taking the JSON output (one object) from each and parsing it for statistics. I'm keeping track of how many processes I've started and capture their output in OutputDataReceived(). I append the output into a ConcurrentBag<object> using the process Id as a key.
Every so often, my JSON ends up as two objects which throws a parsing error. I'm not sure how I end up with data from two different processes in the same object. It's as if the OutputDataReceived() event is getting data from a different process than the Id it is reporting. I tried implementing some locking without any luck (it's a bit new to me as I come from a Classic VB background).
Here's some relevant code:
private object _lockObj = new object();
private ConcurrentBag<ProcData> _procDatas;
// This is called up to X times
private void LaunchProc(int itemId)
{
var proc = new Process();
proc.StartInfo.CreateNoWindow = true;
proc.StartInfo.ErrorDialog = false;
proc.StartInfo.UseShellExecute = false;
proc.StartInfo.RedirectStandardError = true;
proc.StartInfo.RedirectStandardInput = true;
proc.StartInfo.RedirectStandardOutput = true;
proc.EnableRaisingEvents = true;
proc.Exited += proc_Exited;
proc.OutputDataReceived += proc_OutputDataReceived;
proc.ErrorDataReceived += proc_ErrorDataReceived;
proc.StartInfo.WindowStyle = ProcessWindowStyle.Hidden;
proc.StartInfo.FileName = "someapp.exe";
proc.StartInfo.Arguments = "/id=" + itemId;
proc.Start();
proc.BeginErrorReadLine();
proc.BeginOutputReadLine();
}
// I assume I'm screwing something up here since this is the only place where I set OutputData
void proc_OutputDataReceived(object sender, DataReceivedEventArgs e)
{
var proc = sender as System.Diagnostics.Process;
if (proc == null) return;
if (e.Data == null) return;
lock (_lockObj)
{
var item = _procDatas.FirstOrDefault(pi => pi.Id == proc.Id);
if (item == null)
_procDatas.Add(new ProcData() {Id = proc.Id, OutputData = e.Data});
else
item.OutputData += e.Data;
}
}
void proc_Exited(object sender, EventArgs e)
{
var proc = sender as System.Diagnostics.Process;
ProcMessage procMsg = null;
lock (_lockObj)
{
var procInfo = _procDatas.FirstOrDefault(pi => pi.Id == proc.Id);
// JSON is parsed here and error is thrown because of multiple objects (eg {"ProcessId":1,"Msg":"Success"}{"ProcessId":2,"Msg":"Success"})
procMsg = new ProcMessage(procInfo.OutputData);
}
}
public class ProcData
{
public int Id { get; set; }
public string OutputData { get; set; }
public string ErrorData { get; set; }
}
Thanks in advance for any help resolving this issue or suggesting a different (better) approach.
The ID of a Process can get reused. Therefore I recommend not using the process ID as an identifier after the process has exited.
Instead, you could use your itemId as an identifier, associating the process output with it and encapsulating the process and itemId in some container, for instance (tested lightly, seems OK):
public class ProcessExitedEventArgs<TKey> : EventArgs
{
public ProcessExitedEventArgs(TKey key, string[] output)
{
this.Key = key;
this.Output = output;
}
public TKey Key { get; private set; }
public string[] Output { get; private set; }
}
public delegate void ProcessExitedEventHandler<TKey>(object sender, ProcessExitedEventArgs<TKey> e);
public class ProcessLauncher<TKey>
{
public string FileName { get; private set; }
public string Arguments { get; private set; }
public TKey Key { get; private set; }
object locker = new object();
readonly List<string> output = new List<string>();
Process process = null;
bool launched = false;
public ProcessLauncher(string fileName, string arguments, TKey key)
{
this.FileName = fileName;
this.Arguments = arguments;
this.Key = key;
}
public event ProcessExitedEventHandler<TKey> Exited;
public bool Start()
{
lock (locker)
{
if (launched)
throw new InvalidOperationException();
launched = true;
process = new Process();
process.StartInfo.CreateNoWindow = true;
process.StartInfo.ErrorDialog = false;
process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardError = true;
process.StartInfo.RedirectStandardInput = true;
process.StartInfo.RedirectStandardOutput = true;
process.EnableRaisingEvents = true;
process.Exited += new EventHandler(proc_Exited);
process.OutputDataReceived += proc_OutputDataReceived;
process.ErrorDataReceived += proc_ErrorDataReceived;
process.StartInfo.WindowStyle = ProcessWindowStyle.Hidden;
process.StartInfo.FileName = FileName;
process.StartInfo.Arguments = Arguments;
try
{
var started = process.Start();
process.BeginErrorReadLine();
process.BeginOutputReadLine();
return started;
}
catch (Exception)
{
process.Dispose();
process = null;
throw;
}
}
}
void proc_ErrorDataReceived(object sender, DataReceivedEventArgs e)
{
if (e.Data != null)
{
// Fill in as appropriate.
Debug.WriteLine(string.Format("Error data received: {0}", e.Data));
}
}
void proc_OutputDataReceived(object sender, DataReceivedEventArgs e)
{
if (e.Data == null)
return;
lock (locker)
{
output.Add(e.Data);
}
}
void proc_Exited(object sender, EventArgs e)
{
lock (locker)
{
var exited = Exited;
if (exited != null)
{
exited(this, new ProcessExitedEventArgs<TKey>(Key, output.ToArray()));
// Prevent memory leaks by removing references to listeners.
Exited -= exited;
}
}
var process = Interlocked.Exchange(ref this.process, null);
if (process != null)
{
process.OutputDataReceived -= proc_OutputDataReceived;
process.ErrorDataReceived -= proc_ErrorDataReceived;
process.Exited -= proc_Exited;
process.Dispose();
}
}
}
(This class also makes sure the process is disposed.) Then use it like:
public void LaunchProc(int itemId)
{
var launcher = new ProcessLauncher<int>("someapp.exe", "/id=" + itemId, itemId);
launcher.Exited += launcher_Exited;
launcher.Start();
}
void launcher_Exited(object sender, ProcessExitedEventArgs<int> e)
{
var itemId = e.Key;
var output = e.Output;
// Process output and associate it to itemId.
}