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

out_forward: fix error of ack handling conflict on stopping with require_ack_response enabled #4030

Merged
merged 7 commits into from
Feb 10, 2023

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Jan 27, 2023

Which issue(s) this PR fixes:
Fixes #3962

What this PR does / why we need it:
out_forward has a thread always checking the ack and closing the socket.

When stopping Fluentd, some other threads try to do it at the same time.

This somtimes causes errors.
Although these errors are harmless (at least as far as I can confirm), we should fix this.

This fixes 2 points.

  1. Suppress the error log.
    • When another thread closes the socket, there is no need to output an error log.
  2. Fix an unhandled error of ForwardOutput::ach_check().

The logs will change as follows.

  • before:
[info]: #0 fluent/log.rb:330:info: shutting down output plugin type=:forward plugin_id="object:ba4"
[info]: #0 fluent/log.rb:330:info: delayed_commit_timeout is overwritten by ack_response_timeout
[error]: #0 fluent/log.rb:372:error: unexpected error while receiving ack message error_class=IOError error="stream closed in another thread"
[error]: #0 fluent/log.rb:372:error: unexpected error while receiving ack error_class=NoMethodError error="undefined method `disable!' for nil:NilClass"
[info]: fluent/log.rb:330:info: Worker 0 finished with status 0
  • after
[info]: #0 fluent/log.rb:330:info: shutting down output plugin type=:forward plugin_id="object:ba4"
[info]: #0 fluent/log.rb:330:info: delayed_commit_timeout is overwritten by ack_response_timeout
[info]: #0 fluent/log.rb:330:info: socket closed while receiving ack response
[info]: fluent/log.rb:330:info: Worker 0 finished with status 0

Docs Changes:
Not needed.

Release Note:
Same with the title.

Fujimoto Seiji and others added 3 commits January 27, 2023 15:24
When AckHandler fails to collect a response, it eventually yields
Results::FAILED with nulls, but ack_check() was not written carefully
enough to notice the case.

This was the reason why ack_check() somtimes crashed. Fix it by adding
nil checks to methods calls.

Signed-off-by: Fujimoto Seiji <[email protected]>
In the threaded ack receiving case, we must be sure to check
IOError when doing socket operation, in order not to crash on
socket closed by another thread.

Signed-off-by: Fujimoto Seiji <[email protected]>
@ashie ashie self-requested a review January 27, 2023 10:10
@daipom
Copy link
Contributor Author

daipom commented Jan 27, 2023

The test added in this PR seems to be unstable. I will check it.

@daipom
Copy link
Contributor Author

daipom commented Jan 30, 2023

https://github.com/fluent/fluentd/actions/runs/4023398771/jobs/6914222464#step:6:45

I checked this failing test and I found another conflict that we should take care of.
The conflict scenario is as follows.

Signed-off-by: Daijiro Fukuda <[email protected]>
Signed-off-by: Daijiro Fukuda <[email protected]>
@daipom
Copy link
Contributor Author

daipom commented Jan 30, 2023

I have made additional fixes.
The current failing tests seem to have nothing to do with this fix.

@ashie ashie added this to the v1.16.0 milestone Feb 9, 2023
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

LGTM
I commented a nitpick.

lib/fluent/plugin/out_forward.rb Outdated Show resolved Hide resolved
Signed-off-by: Daijiro Fukuda <[email protected]>
@ashie ashie merged commit bf7498d into fluent:master Feb 10, 2023
@ashie
Copy link
Member

ashie commented Feb 10, 2023

Thanks!

@daipom daipom deleted the out-forward-conflict-in-ack-handling branch February 10, 2023 04:21
@daipom
Copy link
Contributor Author

daipom commented Feb 10, 2023

Thanks for your review!

@daipom daipom changed the title out_forward: fix error of ack handling confict on stopping with require_ack_response enabled out_forward: fix error of ack handling conflict on stopping with require_ack_response enabled Mar 29, 2023
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.

out_forward errors on shutdown with require_ack_response enabled
2 participants