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 possible endless loop while polling curl socket #5589

Merged
merged 1 commit into from
May 16, 2024

Conversation

CurtizJ
Copy link
Contributor

@CurtizJ CurtizJ commented May 1, 2024

In case of high frequency of received signals (more than once per second) the field now will never be updated and it can lead to exceeding of timeout or endless loop if socket will never be ready. For instance in ClickHouse signals SIGUSR1 and SIGUSR2 are being sent each second by internal query profiler and we faced this issue.

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

@github-actions github-actions bot added Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels May 1, 2024
Copy link

github-actions bot commented May 1, 2024

Thank you for your contribution @CurtizJ! We will review the pull request and get back to you soon.

Copy link
Member

@LarryOsterman LarryOsterman left a comment

Choose a reason for hiding this comment

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

Oops. Extremely good catch.

Is it possible to write a test for this?

@CurtizJ
Copy link
Contributor Author

CurtizJ commented May 2, 2024

Yes, I'll try to add a test.

@ahsonkhan
Copy link
Member

ahsonkhan commented May 6, 2024

That's a great catch! I am curious, how did you discover this? Did you actually hit the endless loop, in your use case?

@CurtizJ
Copy link
Contributor Author

CurtizJ commented May 13, 2024

how did you discover this?

We use this sdk for integration with azure blob storage in ClickHouse. I ran a stress test with heavy queries and after finishing it I noticed some hung queries with stacktrace on the line changed in this PR. I don't know why poll hasn't received any events, maybe because the connection was by timeout, but I didn't investigate it. After fixing this the issue has disappeared.

@CurtizJ
Copy link
Contributor Author

CurtizJ commented May 13, 2024

Sorry for the delay, it appeared to be harder to add test than I thought.

Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

@LarryOsterman given this is a great reliability bug fix, I propose we merge it.

I'll make a changelog update PR, for core, attributing @CurtizJ for fixing this issue. Thanks!

@ahsonkhan ahsonkhan merged commit 5d3f955 into Azure:main May 16, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants