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

Forwardport PR #16671 to main: PipelineBusV2 deadlock proofing #16682

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 18, 2024

Forwardport PR #16671 to main branch, original message:


Release notes

  • Fixes an issue where Logstash shutdown could stall in some cases when using pipeline-to-pipeline.

What does this PR do?

  • Adds tests to detect deadlock betweeen PipelineBus#unlisten and PipelineBus#unregisterSender
  • Eliminates a deadlock that can occur in PipelineBusV2

Why is it important/What is the impact to the user?

Failing to shut down is not good.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

Related issues

@yaauie yaauie changed the title Backport PR #16671 to main: PipelineBusV2 deadlock proofing Forwardport PR #16671 to main: PipelineBusV2 deadlock proofing Nov 18, 2024
Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

CI failure unrelated. Filed https://github.com/elastic/logstash/pull/16695/files to address that. This port is clean and ready to go.

* pipeline bus: add deadlock test for unlisten/unregisterSender

* pipeline bus: eliminate deadlock

Moves the sync-to-notify out of the `AddressStateMapping#mutate`'s effective
synchronous block to eliminate a race condition where unlistening to an address
and unregistering a sender could deadlock.

It is safe to notify an AddressState's attached input without exclusive access
to the AddressState, because notifying an input that has since been disconnected
is net zero harm.

(cherry picked from commit 8af6343)
@jsvd
Copy link
Member

jsvd commented Nov 20, 2024

I've rebased the PR against main, should be green soon.

@donoghuc
Copy link
Member

Ah, I was just about to ask about that. I had tried to reload the failed cell thinking maybe it would try to test changes against the current head of main. Seems like instead it just tests based on the head of the branch being tested.

Copy link

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@jsvd
Copy link
Member

jsvd commented Nov 20, 2024

🍏 🟢 💚 📗

@donoghuc donoghuc merged commit 0dd64a9 into main Nov 20, 2024
6 checks passed
@donoghuc donoghuc deleted the backport_16671_main branch November 21, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PipelineBusV2 block shutdown
4 participants