search

libcpr / cpr

C++ Requests: Curl for People, a spiritual port of Python Requests.
https://docs.libcpr.org/
Other
6.29k stars 903 forks source link

Made interceptors runnable for any number of requests on a single Session #1038

Closed AndreGleichner closed 2 months ago

AndreGleichner commented 2 months ago

Fixing #1036 Didn't implement noticeable tests yet until I know this fix would be acceptable or goes into the right direction.

COM8 commented 2 months ago

@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.

AndreGleichner commented 2 months ago

Fixed clang-tidy. Do you have any specific needs/wishes for more unit tests?

COM8 commented 2 months ago

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.

AndreGleichner commented 2 months ago

@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 https://github.com/libcpr/cpr/commit/36409bcb3e207955621fb6b2295d296052a68bc2

COM8 commented 2 months ago

@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.

COM8 commented 2 months ago

https://github.com/libcpr/cpr/issues/1047