Refactor HTTP client session handling; add curl debug logs #1870
Conversation
|
Yes, I agree that it would be good to get curl out of the header, and I also think the _new() and _free() for the session would be good. In general, I think the goal of the logs is to be able to receive data from users to determine what was going on when an issue occured, so that it can be reproduced. They are not for when developing/debugging yourself. So I think logging the contents of every request on debug is too noisy. It's enough to know that OwnTone tried to make a request and the result of that. That's already logged today. On SPAM level I think logging everything is ok, it's spam after all. I also don't agree with the mutex. The implementation doesn't use any mutable globals as far as I can see, so it's already thread safe. Of course there will be problems if two threads call the function with a reference to the same session, but then it's the caller that should protect with a mutex, not the implementation. I would say it's a standard assumption for any function implementation that the input given by caller isn't going to become invalid during function execution.. I see you also removed "headers_only", since it's not used any more. I think that's fine, but maybe a note about it (removing it since not used any more) in the commit message. |
Depends on the kind of error. For connection errors it would be good to have logs for the actual connection attempt.
Well, the implementation modifies the curl handle (that is an implementation detail for the caller) which in turn makes this call not thread safe. Forcing each caller to guard the call with a mutex, makes the api more difficult to use and results in code duplication.
In this case the implementation makes the the input invalid for other calls for the same session. For example, if libcurl would be replaced by a different library, the method itself might become thread safe. Then all callers should remove the guard with the mutex. If you insist, I'll move guarding with a mutex to the caller, but in my opinion, it makes more sense to simplify the code and keep the details hidden from the caller. It might also be worth to have a default session or to at least share the caches:
Totally forgot, that I removed it ... will update the commit message. |
That's fine, but I'm not sure who would be using it? Regular users won't recompile, and I suppose you and me and others who work with the code use either the debugger and/or add ad hoc logging when troubleshooting.
Many functions modify memory pointed to by an argument, I wouldn't say that makes them not thread safe. By that definition sprintf() or strtok_r() are not thread safe? Another reason to have the mutex in caller scope is that only the caller can know if locking is actually needed. There is no reason to do any locking for all the regular one-thread/one-off requests. Just like locking for sprintf() would only be needed if two threads both do sprintf(x), which is something the implementation can't know. So I still think it should be moved out of the implementation. |
First commit contains a small refactoring to make reusing curl handles easier to use (with the
struct http_client_session). Thread safety is now ensured byhttp_client_request(...)and the implementation details ofstruct http_client_sessionare now hidden, therefor the include of<curl.h>is now gone fromhttp.h.The second commit adds debug / spam logging for libcurl. Info messages, request/response headers are now logged with level DEBUG, request/response body and SSL data are logged with level SPAM.
Seeing the actual requests and responses made by libcurl can be very helpful when analyzing issues.
Example logs: