Skip to content

Fix Input and Output deadlock when buffer is full during startup#1502

Merged
repeatedly merged 3 commits into
masterfrom
fix-deadlock-for-input-start
Mar 15, 2017
Merged

Fix Input and Output deadlock when buffer is full during startup#1502
repeatedly merged 3 commits into
masterfrom
fix-deadlock-for-input-start

Conversation

@repeatedly

@repeatedly repeatedly commented Mar 15, 2017

Copy link
Copy Markdown
Member

Output threads need calling after_start to run but
after_start is sometimes not called when Input#start is blocked.
This happens when in_tail with read_from_head true and
output with overflow_action block.
This fixes the problem by checking after_start immediately after start.

This may be related with #1485

@repeatedly
Fix Input and Output deadlock when buffer is full during startup
a4497b0 
Output threads need calling `after_start` to run but
`after_start` is sometimes not called when Input#start is blocked.
This happens when in_tail with `read_from_head true` and
output with `overflow_action block`.
This fixes the problem by checking `after_start` immediately after start.
@repeatedly repeatedly requested a review from tagomoris March 15, 2017 02:37
@repeatedly

repeatedly commented Mar 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@tagomoris Do you see any concern for this patch?
after_start should be called after all start call?

@repeatedly repeatedly mentioned this pull request Mar 15, 2017
Closed
@tagomoris

tagomoris commented Mar 15, 2017

Copy link
Copy Markdown
Member

@repeatedly this change breaks many tests (makes tests unstable at least). read this commit comment: bd5841e

IMO it's wrong to emit events in TailInput#start. It should be done in another threads executed from #start thread.

@repeatedly

repeatedly commented Mar 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

this change breaks many tests (makes tests unstable at least).

Which point? input or filter start assumes Output#after_start is not called?
From the commit, Output#after_start is for checking Output#start is finished or not. This doesn't depend on other plugin status.

IMO it's wrong to emit events in TailInput#start. It should be done in another threads executed from #start thread.

Yes but current plugin API, it is hard, right?
How to know other threads are running inside input plugin?

@tagomoris

tagomoris commented Mar 15, 2017

Copy link
Copy Markdown
Member

About the order of #start and #after_start, I made wrong comment. As you mentioned, there are no barrier to call #after_start immediately after calling #start per plugins.

But it depends the order of RootAgent#lifecycle(desc: true) why this patch solves #1485. So the comment in root_agent.rb should mention that desc: true means the order from Output, Filter, Output with event_emitter and Input.

About the threading, there are some options to do it - running long-running process in #start is bad practice anyway.

  1. adding immediate: (default false) option to PluginHelper::Timer#timer_create to invoke callback immediately in timer thread (by creating TimerWatcher with 0 interval and repeat: false)
  2. using thread_create to run refresh_watchers in TailInput#start

The first option is more general solution. ChildProcess#child_process_execute method also has immediate: optional argument besides interval: argument, so having same option in timer_create looks consistent solution. (in that case, child_process_execute(immediate: true) can be re-implemented on timer_create(immediate: true)).

The second option is to call thread_create and call refresh_watchers by TailInput itself. It may call #refresh_watchers in parallel from the thread created and timer threads without any control, so it requires mutex to prohibit it.

@repeatedly

repeatedly commented Mar 15, 2017

Copy link
Copy Markdown
Member Author

adding immediate: (default false) option to PluginHelper::Timer#timer_create to invoke callback immediately in timer thread (by creating TimerWatcher with 0 interval and repeat: false)

It seems good. I will implement this feature for in_tail like case in other patch.
in_tail specific fix is not good for plugin authors. So 2 approach is less merit.

@repeatedly

repeatedly commented Mar 15, 2017

Copy link
Copy Markdown
Member Author

Update comment of RootAgent#start

tagomoris
tagomoris reviewed Mar 15, 2017
View reviewed changes
Comment thread lib/fluent/root_agent.rb Outdated
# buffer related output threads should be run before `Input#start`.
# This is why after_start should be called immediately after start call.
# This depends on `desc: true` because calling plugin order of `desc: true` is
# Output, Filter, Label, Output with Router and Input.

@tagomoris tagomoris Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Output with Router, then Input" looks better to emphasis the order.

tagomoris
tagomoris approved these changes Mar 15, 2017
View reviewed changes

@tagomoris tagomoris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a trivial comment. LGTM.

@tagomoris

tagomoris commented Mar 15, 2017

Copy link
Copy Markdown
Member

Ah, please open an issue to add the immediate option to #timer_create before closing this pull-request.

@repeatedly repeatedly mentioned this pull request Mar 15, 2017
Open
@repeatedly repeatedly force-pushed the fix-deadlock-for-input-start branch from cb8a2ce to ecf0a6a Compare March 15, 2017 19:32
@repeatedly repeatedly merged commit d98dd81 into master Mar 15, 2017
@repeatedly repeatedly deleted the fix-deadlock-for-input-start branch March 15, 2017 19:34
@ashie ashie mentioned this pull request May 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tagomoris tagomoris tagomoris approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@repeatedly @tagomoris