> On Dec. 16, 2016, 7:05 p.m., Vadim Spector wrote: > > It is probably ok, but ... > > a) Did you investigate why service stayed alive? Perhaps, some knowledge > > about the implementation and its defficiencies to be gained there. Some > > unaccounted runaway threads? Or are we unable to termonaie some threads? > > b) System.exit() means that Sentry service better be the only thing running > > in JVM. Which I presume is the case now. Are we ok with making that a > > requirement from now on? > > Alexander Kolbasov wrote: > For a) please see SENTRY-1526. The service stayed alive because of the > deadlock. > b) I don't understand the question - what else can run in this JVM? This > is a Sentry Server that we are trying to kill. > > Vadim Spector wrote: > a) Can the deadlock be fixed instead? SENTRY-1526 seem to suggest that > the choice of synchronization objects can be changed to prevent the deadlock. > It still does not explain why we need to use System.exit(). > b) Architecturally, there is nothing insane about running Sentry Service > embedded inside some kind of container in the future. > We may not need it now, but the proposed implementation makes this > impossible moving forward, therefore my concern.
Regarding (a). This is not a classic deadlock (which means two threads holding two separate locks). Here we have something simpler: thread A preventing thread B from proceeding by holding a single lock unnecessarily, by calling synchronized method waitOnFutures(). If you look at its code and how it's used, you will immediately see that this method just doesn't need to be synchronized. Sasha fixed that by removing the whole waitOnFutures() method altogether and moving its code (a single line) into the only caller of waitOnFutures(). - Misha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54808/#review159478 ----------------------------------------------------------- On Dec. 16, 2016, 7:23 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54808/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2016, 7:23 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > SENTRY-1526 Sentry processed stays alive after being killed > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 578364933a3cdcf6c142b836360a83d322fe5c11 > > Diff: https://reviews.apache.org/r/54808/diff/ > > > Testing > ------- > > Now process successfully dies after hitting ^C. > > > Thanks, > > Alexander Kolbasov > >
