Skip to content
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

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

Merged
merged 3 commits into from
Mar 15, 2017

Conversation

repeatedly
Copy link
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

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
Copy link
Member Author

repeatedly commented Mar 15, 2017

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

@tagomoris
Copy link
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
Copy link
Member Author

repeatedly commented Mar 15, 2017

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
Copy link
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
Copy link
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
Copy link
Member Author

Update comment of RootAgent#start

# 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.
Copy link
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.

Copy link
Member

@tagomoris tagomoris left a comment

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
Copy link
Member

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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants