azure-sdk-for-cpp icon indicating copy to clipboard operation
azure-sdk-for-cpp copied to clipboard

Fix possible endless loop while polling curl socket

Open CurtizJ opened this issue 9 months ago • 2 comments

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.

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

CurtizJ avatar May 01 '24 11:05 CurtizJ

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

github-actions[bot] avatar May 01 '24 11:05 github-actions[bot]

Yes, I'll try to add a test.

CurtizJ avatar May 02 '24 20:05 CurtizJ

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

ahsonkhan avatar May 06 '24 19:05 ahsonkhan

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 avatar May 13 '24 13:05 CurtizJ

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

CurtizJ avatar May 13 '24 13:05 CurtizJ