-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FLUME-3056 TestApplication hangs indefinitely #108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks for the fix. I had one minor comment, otherwise 👍
supervisor.stop(); | ||
if (monitorServer != null) { | ||
monitorServer.stop(); | ||
} | ||
stopping = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patch @andrasbeni, it looks good to me, 👍
Retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it with putting a sleep in the file watcher to make sure that the application object is locked while shutdown.
Without the fix, the agent hangs, with the fix it stops successfully.
…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