I have a program that processes Microsoft Office documents (and other kinds, but primarily Office). I am replacing one purchased product that performs this processing with a new one. This new product consists of a library which is linked in to my code and a Windows service, which performs the actual work. For the most part this new product works better than the old one, but it has a flaw in how it handles some Office documents. Documents that are password-protected can't be processed, and furthermore the third-party product never returns from processing them. For my code this is not a problem; I run the call to the processing on a separate thread with a timeout. After a while I give up. Unfortunately the third-party Windows service does not, and it turns out that it has only a limited number of processing slots. If I send too many documents that don't finish processing, the service stops processing new requests.
I have developed a "hack" to work around this flaw. All requests for processing are sent as lambdas to the class I show below. The requests are multi-threaded at a higher level, so there may be more than one request processing at any time. If a request fails (either by returning false or by throwing an exception), the class stops processing new requests by having threads wait on a manual reset event and waits for threads that are currently processing to finish. Once there are no more running requests for processing, the class calls the code that restarts the service. Once the reset finishes, the class releases any threads waiting on the manual reset event.
There are a number of points on which I am looking for review. First, I have concerns about the correctness of the multi-threaded code. There is one "flaw" that I know about. If any of the passed-in lambdas go into an infinite loop, the reset won't work. This is mitigated in the rest of the code where the lambdas have a "timed executor" wrapper around themselves. But I could have made the timed executor part of the reset class, and I wonder if I should have.
Second, see if it follows SOLID principles. For instance, I wonder if the class is "too big." I can't think of any way I could split it, but it looks a bit big to me.
Third I am unhappy about how the reset action and reset fail action lambdas are set, because they can be accessed by any code at any time. In my code these are set only once, but there's nothing preventing them from being reset by other code. Because this is a singleton, the constructor is private and called in the class, so the obvious solution of passing in these lambdas on the constructor won't work. I don't have any good ideas on how to solve this, if in fact it needs solving.
Bonus: I wish the class had a better name.
Note, his was written for .NET version 2.0. Because of that, I have defined two delegates that are part of later versions of .NET, Action() and Func().
public class CoordinatedActionWithReset {
private static CoordinatedActionWithReset() {
_coordinatedActionWithReset = new CoordinatedActionWithReset();
}
private readonly object _coordinatedActionWithResetLock = new object();
private int _countCurrentlyExecuting;
private bool _resetRequested;
private readonly ManualResetEvent _waitForReset = new ManualResetEvent(true);
private static CoordinatedActionWithReset _coordinatedActionWithReset;
private CoordinatedActionWithReset() {
ResetAction = () => { };
ResetFailAction = (ex) => { };
}
public static CoordinatedActionWithReset Instance {
get { return _coordinatedActionWithReset; }
}
public delegate void Action();
public delegate T Func<T>();
public Action ResetAction { get; set; }
public Action<Exception> ResetFailAction { get; set; }
private void RequestReset() {
lock (_coordinatedActionWithResetLock) {
_resetRequested = true;
_waitForReset.Reset();
CheckForReset();
}
}
public bool ExecuteAction(Func<bool> action) {
DelayIfResetPending();
bool goodResult;
try {
goodResult = action();
if (!goodResult) {
RequestReset();
}
}
catch {
goodResult = false;
RequestReset();
}
finally {
EndAction();
}
return goodResult;
}
private void DelayIfResetPending() {
var succeeded = false;
while (!succeeded) {
_waitForReset.WaitOne();
lock (_coordinatedActionWithResetLock) {
if (!_resetRequested) {
_countCurrentlyExecuting++;
succeeded = true;
}
}
}
}
private void EndAction() {
lock (_coordinatedActionWithResetLock) {
_countCurrentlyExecuting--;
CheckForReset();
}
}
private void CheckForReset() {
if (_resetRequested && _countCurrentlyExecuting == 0) {
Reset();
}
}
private void Reset() {
try {
ResetAction();
}
catch (Exception ex) {
try {
ResetFailAction(ex);
}
catch { }
}
finally {
_resetRequested = false;
_waitForReset.Set();
}
}
}
