fix(rest): support rewinds in libcurl #11703
Conversation
We configure libcurl to get the data for PUT and POST requests using callbacks. Sometimes libcurl may have sent part of the data and needs to resend it. The documentation mentions "multi-pass authentication methods", as well as reusing connections where libcurl detects they were closed after some data is sent. There are cases on the Internet about chasing HTTP redirects (3xx responses) too. In any case, we have seen these as flakes in the tests. I took the belt and suspenders approach. Try to handle the rewind requests, but if unhandled treat the error as retryable, because they are.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11703 +/- ##
==========================================
- Coverage 83.03% 83.02% -0.02%
==========================================
Files 1827 1827
Lines 164704 164734 +30
==========================================
+ Hits 136763 136770 +7
- Misses 27941 27964 +23
☔ View full report in Codecov by Sentry. |
| writev_.assign(original_.begin(), original_.end()); | ||
| // Reverse the vector so the first chunk is at the end. | ||
| std::reverse(writev_.begin(), writev_.end()); |
There was a problem hiding this comment.
We lost the std::reverse() in the initialization of writev_, but we do one here in its re-initialization. That seems inconsistent.
There was a problem hiding this comment.
I assumed it was better to not reverse in the case of read. 🤷
There was a problem hiding this comment.
The reverse was originally so that vector::pop_back() was used to consume data, but with the move to deque that is less of a consideration. Still, it seems (without looking deeply) that it should be all or none.
There was a problem hiding this comment.
Ooops I will send a fix in a few minutes.
| }; | ||
|
|
||
| extern "C" { // libcurl callbacks |
There was a problem hiding this comment.
Aside: I wasn't sure exactly how extern "C"-within-namespace {} was defined, so I put it outside, but I guess if it works (with our compilers) then all is fine.
There was a problem hiding this comment.
Yeah, it works, but effectively makes the symbols have external linkage:
https://stackoverflow.com/questions/35288838/external-linkage-for-name-inside-unnamed-namespace
I will revert that change too.
We configure libcurl to get the data for PUT and POST requests using callbacks. Sometimes libcurl may have sent part of the data and needs to resend it. The documentation mentions "multi-pass authentication methods", as well as reusing connections where libcurl detects they were closed after some data is sent. There are cases on the Internet about chasing HTTP redirects (3xx responses) too. In any case, we have seen these as flakes in the tests.
I took the belt and suspenders approach. Try to handle the rewind requests, but if unhandled treat the error as retryable, because they are.
Fixes #11167
This change is