Re: [hibernate-dev] Hibernate Search 421 [Exceptions happening in backend are unnoticed]

Hi Amin, I've been looking a bit in this, but didn't take any action as we didn't discuss any strategy, so glad you join in propose something. I didn't think of jms, it's ok we provide some way for the end user to override whatever we provide, but I think we should just provide the basic stuff: logging the error or propagate it back when possible. And then add some extensibility like plugin loading the usual way. Originally (Hibernate Search 3.0.x) the changes made to different indexes were serialized, so when using the "sync" backend the same process would have applied changes to the index and, in case of exceptions, this would have been propagated up to user's code making it impossible to go unnoticed, or at least moving responsibility of handling it to the developer. When using the "async" mode for backend a different thread would have handled the indexwriter, so in case of an exception the error would have killed the separate thread, go unnoticed, and as we use a ThreadPool a new thread would have been spawned to replace the failed one. Since Search 3.1.x both "sync" and "async" use a separate thread - to be able to apply changes to different indexes in parallel - so exceptions are going unnoticed by user code even in "sync" mode. Look into "run" method of org.hibernate.search.backend.impl.lucene.LuceneBackendQueueProcessor, it creates a org.hibernate.search.backend.impl.lucene.QueueProcessors where most of this logic resides. This impl will use either runAllAsync() or runAllWaiting() depending on backend configuration (async/sync). The Async version just schedules the different tasks, the sync one is going to do the same but will wait for all of them to have finished before returning control, making sure to implement the sync behaviour even while using several threads to perform it. I was inspired by java.util.concurrent.AbstractExecutorService.invokeAll(Collection<Callable<T>> tasks), as mentioned in code comments, which is ignoring ExecutionException, so this empty catch block should be used now to do something. I'd suggest to provide this defaults: * for sync backend: by default: rethrow the exception (like old behaviour) configurable alternative: log it * for async backend: log it (it's not useful to rethrow as nobody is listening on it) To handle these cases only you would add the log statement, an rethrow it, fixing the QueueProcessors code. To give full control to the end user of what to do, I'd suggest to let him specify an implementationf of java.lang.Thread.UncaughtExceptionHandler and so we can set that on the thread. The backend is using the default JVM "Executors.newFixedThreadPool( 1 );" (initialized at org.hibernate.search.backend.impl.lucene.PerDPResources:51) but we could change that to use the Search Executor factory "org.hibernate.search.batchindexing.Executors" I later added for the batchbackend. The nice thingy for this Executor is that it customizes the thread names so you can easily spot Search's threads in monitoring/debugging tools; we could have a setUncaughtExceptionHandler( userImplementation ) in org.hibernate.search.batchindexing.Executors:85. If you change it there, the same handler would be used to manage errors in the batch indexer, so you solve both problems at once. Our "log the error" implementation would be the nice default for an exception handler, but still I'd like to make sure the user code will get the error propagated when using async mode. Cheers, Sanne 2010/1/9 Amin Mohammed-Coleman <aminmc(a)gmail.com&gt;: