c# - Nested lock in Task.ContinueWith - Safe, or playing with fire? -

windows service: generating set of filewatcher objects list of directories watch in config file, have following requirements:

1. file processing can time consuming - events must handled on own task threads
2. keep handles event handler tasks wait completion in onstop() event.
3. track hashes of uploaded files; don't reprocess if not different
4. persist file hashes allow onstart() process files uploaded while service down.
5. never process file more once.

(regarding #3, events when there no changes... notably because of duplicate-event issue filewatchers)

to these things, have 2 dictionaries - 1 files uploaded, , 1 tasks themselves. both objects static, , need lock them when adding/removing/updating files , tasks. simplified code:

public sealed class trackingfilesystemwatcher : filesystemwatcher {      private static readonly object filewatcherdictionarylock = new object();     private static readonly object runningtaskdictionarylock = new object();      private readonly dictionary<int, task> runningtaskdictionary = new dictionary<int, task>(15);     private readonly dictionary<string, filesystemwatcherproperties>  filewatcherdictionary = new dictionary<string, filesystemwatcherproperties>();      //  wired elsewhere     private void onchanged(object sender, filesystemeventargs eventargs) {         this.processmodifieddatafeed(eventargs);     }      private void processmodifieddatafeed(filesystemeventargs eventargs) {          lock (trackingfilesystemwatcher.filewatcherdictionarylock) {              //  read file , generate hash here              //  properties if file has been processed before             //  containsnonnullkey extension method             if (this.filewatcherdictionary.containsnonnullkey(eventargs.fullpath)) {                  try {                     fileproperties = this.filewatcherdictionary[eventargs.fullpath];                 }                 catch (keynotfoundexception keynotfoundexception) {}                 catch (argumentnullexception argumentnullexception) {}             }             else {                   // create new properties object             }               fileproperties.changetype = eventargs.changetype;             fileproperties.filecontentshash = md5hash;             fileproperties.lasteventtimestamp = datetime.now;              task task;             try {                 task = new task(() => new datafeeduploadhandler().uploaddatafeed(this.legalorg, datafeedfiledata), taskcreationoptions.longrunning);             }             catch {               ..             }              //  lock long enough add task dictionary             lock (trackingfilesystemwatcher.runningtaskdictionarylock) {                  try {                     this.runningtaskdictionary.add(task.id, task);                   }                 catch {                   ..                 }                 }               try {                 task.continuewith(t => {                     try {                         lock (trackingfilesystemwatcher.runningtaskdictionarylock) {                             this.runningtaskdictionary.remove(t.id);                         }                          //  lock burn me?                         lock (trackingfilesystemwatcher.filewatcherdictionarylock) {                             //  persist file watcher properties                             //  disk recovery @ onstart()                         }                     }                     catch {                       ..                     }                 });                  task.start();             }             catch {               ..             }           }      }  } 

what's effect of requesting lock on filesystemwatcher collection in continuewith() delegate when delegate defined within lock on same object? expect fine, if task starts, completes, , enters continuewith() before processmodifieddatafeed() releases lock, task thread suspended until creating thread has released lock. want make sure i'm not stepping on delayed execution landmines.

looking @ code, may able release lock sooner, avoiding issue, i'm not yet... need review full code sure.

update

to stem rising "this code terrible" comments, there reasons why catch exceptions do, , catching many of them. windows service multi-threaded handlers, , may not crash. ever. if of threads have unhandled exception.

also, exceptions written future bulletproofing. example i've given in comments below adding factory handlers... code written today, there never null task, if factory not implemented correctly, code throw exception. yes, should caught in testing. however, have junior developers on team... "may. not. crash." (also, must shut down gracefully if there is unhandled exception, allowing currently-running threads complete - unhandled exception handler set in main()). have enterprise-level monitors configured send alerts when application errors appear on event log – exceptions log , flag us. approach deliberate , discussed decision.

each possible exception has each been considered , chosen fall 1 of 2 categories - apply single datafeed , not shut down service (the majority), , indicate clear programming or other errors fundamentally render code useless datafeeds. example, we've chosen shut down service down if can't write event log, that's our primary mechanism indicating datafeeds not getting processed. exceptions caught locally, because local context place decision continue can made. furthermore, allowing exceptions bubble higher levels (1) violates concept of abstraction, , (2) makes no sense in worker thread.

i'm surprised @ number of people argue against handling exceptions. if had dime every try..catch(exception){do nothing} see, you'd change in nickels rest of eternity. argue death¹ if call .net framework or own code throws exception, need consider scenario cause exception occur , explicitly decide how should handled. code catches unauthorizedexceptions in io operations, because when considered how happen, realized adding new datafeed directory requires permissions granted service account (it won't have them default).

i appreciate constructive input... please don't criticize simplified example code broad "this sucks" brush. code not suck - bulletproof, , so.

¹ argue long time if jon skeet disagrees

no not burn you. if continuewith inlined current thread running new task(() => new datafeeduploadhandler().. lock e.g. no dead lock. lock statement using monitor class internally, , reentrant. e.g. thread can aquire lock multiple times if got/owns lock. multithreading , locking (thread-safe operations)

and other case task.continuewith starts before processmodifieddatafeed finished said. thread running continuewith have wait lock.

i consider task.continuewith , task.start() outside of lock if reviewed it. , possible based on posted code.

you should take @ concurrentdictionary in system.collections.concurrent namespace. make code easier , dont have manage locking yourself. doing kind of compare exchange/update here if (this.filewatcherdictionary.containsnonnullkey(eventargs.fullpath)). e.g. add if not in dictionary. 1 atomic operation. there no function concurrentdictionary there addorupdate method. maybe can rewrite using method. , based on code safely use concurrentdictionary @ least runningtaskdictionary

oh , taskcreationoptions.longrunning literally creating new thread every task kind of expensive operation. windows internal thread pool intelligent in new windows versions , adapting dynamically. "see" doing lots of io stuff , spawn new threads needed , practical.

greetings