Skip to content

Close the read pipe at the right moment #1351

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 30, 2023
Merged

Close the read pipe at the right moment #1351

merged 1 commit into from
Jan 30, 2023

Conversation

@matheussilvasantos
Copy link

What does this pull request do?

Closes the read pipe whether the HTTP request thread was executed or not.

Why is it important?

If we only close the read pipe after the HTTP request inside the child thread, a file descriptor leak can happen.

Checklist

  • I have signed the Contributor License Agreement.
  • My code follows the style guidelines of this project (See .rubocop.yml)
  • I have rebased my changes on top of the latest main branch
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation
  • I have updated CHANGELOG.asciidoc
  • I have updated supported-technologies.asciidoc
  • Added an API method or config option? Document in which version this will be introduced

Related issues
@cla-checker-service
Copy link

cla-checker-service bot commented Jan 21, 2023 •
edited
Loading

💚 CLA has been signed
@matheussilvasantos matheussilvasantos force-pushed the fix-file-descriptors-leak branch from 5d237df to a3e6496 Compare January 21, 2023 03:55
@ghost
Copy link

ghost commented Jan 21, 2023 •
edited by ghost
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-01-30T12:49:57.462+0000

  • Duration: 28 min 52 sec

Test stats 🧪

Test Results
Failed 0
Passed 44400
Skipped 79
Total 44479

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)
@matheussilvasantos matheussilvasantos force-pushed the fix-file-descriptors-leak branch from a3e6496 to ec95e4d Compare January 21, 2023 04:01
@estolfo
Copy link
Contributor

estolfo commented Jan 23, 2023

jenkins please test
@matheussilvasantos
Copy link
Author

Hi @estolfo, is there anything I have to fix on my side?
@estolfo
Copy link
Contributor

estolfo commented Jan 24, 2023

Hi @matheussilvasantos thanks for following up. There are some test failures that I need to look at. They don't have to do with this code; it looks like one of the gems we test with dropped support for a version of Ruby we have in our test matrix.
I'll take a look at the test failures and have an update soon, but there's nothing I need from your side right now. Thanks again for your contribution!
@estolfo
Copy link
Contributor

estolfo commented Jan 26, 2023 •
edited
Loading

Hi @matheussilvasantos can you rebase against main now? The test failures have been resolved in the latest commit. Thanks!
@matheussilvasantos
Close the read pipe at the right moment
96bc259 
We can't close the read pipe inside the child thread because it won't always be executed, so in cases where some exception happens and prevent the child thread from being executed, there would be a file descriptor leak. We shall close the read side of the pipe whether we executed the child thread or not.
@matheussilvasantos matheussilvasantos force-pushed the fix-file-descriptors-leak branch from ec95e4d to 96bc259 Compare January 26, 2023 13:20
@matheussilvasantos
Copy link
Author

Hi @estolfo. I've rebased it.
@estolfo
Copy link
Contributor

estolfo commented Jan 27, 2023

jenkins please test
estolfo
estolfo requested changes Jan 27, 2023
View reviewed changes
@matheussilvasantos matheussilvasantos mentioned this pull request Jan 27, 2023
Closed
@ghost
Copy link

ghost commented Jan 30, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 99.231% (129/130) ❕
Classes 99.231% (129/130) ❕
Lines 59.526% (2562/4304) ❕
Conditionals 100.0% (0/0) 💚
@estolfo estolfo merged commit 444f9e0 into elastic:main Jan 30, 2023
@estolfo
Copy link
Contributor

estolfo commented Jan 30, 2023

Thanks @matheussilvasantos !
@matheussilvasantos
Copy link
Author

Thank you too, @estolfo. Do you intend to release it at RubyGems any time soon?
@estolfo
Copy link
Contributor

estolfo commented Jan 31, 2023

@matheussilvasantos we just did a release within the last two weeks so I'm going to wait a little longer before doing another one, given that this is a bit of an edge case.
@matheussilvasantos
Copy link
Author

Hi, again, Emily. Would you have an estimate of when you will release it? This caused issues in production once, and we want to avoid creating a dependency on GitHub by pointing our gem to the repo. Thanks!
@matheussilvasantos matheussilvasantos deleted the fix-file-descriptors-leak branch February 14, 2023 18:04
@estolfo
Copy link
Contributor

estolfo commented Feb 15, 2023

Hi @matheussilvasantos I was hoping to get this issue resolved and in the next release so was holding off. I'll give it a few more days and if it's not resolved by then, I'll do a release including this issue's fix.
@estolfo
Copy link
Contributor

estolfo commented Mar 1, 2023

@matheussilvasantos v4.6.1 is released with this fix. Thanks again!
estolfo pushed a commit that referenced this pull request Mar 2, 2023
@matheussilvasantos @estolfo
Close the read pipe at the right moment (#1351)
552d645 
We can't close the read pipe inside the child thread because it won't always be executed, so in cases where some exception happens and prevent the child thread from being executed, there would be a file descriptor leak. We shall close the read side of the pipe whether we executed the child thread or not.
@matheussilvasantos
Copy link
Author

@estolfo, thank you too!
jclusso added a commit to jclusso/apm-agent-ruby that referenced this pull request Apr 13, 2023
@jclusso
Merge remote-tracking branch 'elastic/main'
e81cec0 
* elastic/main: (30 commits) docs: remove kibana endpoint (elastic#1381) Update status badge (elastic#1379) Create single status check that can be set as required (elastic#1378) Remove jenkins related precommit hooks (elastic#1380) Migrate Jenkinsfile 2 GH Actions Workflow (elastic#1366) Migrate update specs to updatecli (elastic#1375) v4.6.2 Fixing Faraday::RackBuilder::StackLocked (elastic#1371) Fix jruby docker images (elastic#1367) Update reference to sinatra main (elastic#1373) Update release:update_branch task to reference branch 4.x Add missing docs reference v4.6.1 Add security options to docker containers (elastic#1356) Make sure http status code is set when Faraday Middleware is used (elastic#1368) Use composite action for updatecli workflow (elastic#1365) Fix sha source in updatecli update-specs.yml (elastic#1363) Add update-specs updatecli workflow (elastic#1359) use jruby user to run docker containers (elastic#1355) Close the read pipe at the right moment (elastic#1351) ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-ruby

2 participants

@matheussilvasantos @estolfo