-
Notifications
You must be signed in to change notification settings - Fork 947
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
Made interceptors runnable for any number of requests on a single Session #1038
Conversation
…sion object and allow repeating of requests within an interceptor. Repeated requests intentionally only run subsequent interceptors of the interceptor chain as otherwise re-entrance of intercept() would be an issue.
@AndreGleichner thank you very much for taking a look at it! This direction in my eyes looks good and I would be more than happy to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at clang-tidy.
Fixed clang-tidy. Do you have any specific needs/wishes for more unit tests? |
Thanks for making the changes. I will give you a detailed review on Tuesday. Regarding unit tests, I don't think we need any more here. Every all cases (no interceptor, ...) are already handled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing major. Just a few minor things and we are ready to go :)
…auto for intercept() retval. Tried to extend a MultiPerform interceptor test to showcase it can run multiple times, just to find that MultiPerform is not yet able to perform multiple times.
@COM8 done all the remaining changes. Tried to extend a MultiPerform interceptor test to showcase it can run multiple times, just to find that MultiPerform is not yet able to perform multiple times. Took some time to realize its not my code. Reproduced in a plain unmod cpr by just doing a second Get() on the same trivial MultiPerform. It fails in MultiPerform::ReadMultiInfo() where curl_multi_info_read() returns null. I must be doing something wrong as I can't beleave nobody ever used a MultiPerform object do do another Get request or similar. Doen't know if this might a limitation with curl_multi_perform. Another observation: async_tests (AsyncGetMultipleReflectTest) are quite flaky (~1/3). It fails often on my Windows laptop (quite fast Core i9). If I change the loop count from 100 to 10 it seems stable. You did the same in 36409bc |
@AndreGleichner thanks and yes, I can confirm... Just quickly looked into it. This to me seams like a big oversight. I will create an issue for it. |
Fixing #1036
Didn't implement noticeable tests yet until I know this fix would be acceptable or goes into the right direction.