FLUME-3056 TestApplication hangs indefinitely #108
Conversation
| supervisor.stop(); | ||
| if (monitorServer != null) { | ||
| monitorServer.stop(); | ||
| } | ||
| stopping = false; |
There was a problem hiding this comment.
In general it's a good practice to wrap this to a finally block. In this particular case it mightn't be necessary as this is only called from the shutdown hook, so in fact it doesn't matter whether this is set to false. But in that case do we really need this line, wdyt?
There was a problem hiding this comment.
Thanks for your remark. I'm afraid I will have to rework my change a bit, because this single boolean flag will not entirely prevent the issue.
There was a problem hiding this comment.
Hi @andrasbeni,
Could you please explain the issue in detail? What exactly is in deadlock and why this change prevents it to happen?
Just guessing do we suffer from the same issue during start?
| @@ -65,6 +68,7 @@ | |||
| private final LifecycleSupervisor supervisor; | |||
| private MaterializedConfiguration materializedConfiguration; | |||
| private MonitorService monitorServer; | |||
| private volatile Lock lock = new ReentrantLock(); | |||
There was a problem hiding this comment.
I'd choose a more descriptive name for this, e.g. lifecycleLock. I suppose the volatile just remained from the previous change, a final would be better here :)
There was a problem hiding this comment.
Thank you for the patch @andrasbeni, it looks good to me, 👍
|
Retest this please |
…ock when stopped while handleConfigurationEvent Adding better locking mechanism to Application class to prevent deadlock. this closes apache#108 this closes apache#144 Revievers: Denes Arvay, Attila Simon, Benedict Jin, Ferenc Szabo (Andras Beni, Yan Jian via Ferenc Szabo) Change-Id: I7b598337478cbfb2b6b854116202cacd4a40ef0a
Skip configuration handling in Application while stop() is running