-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
HTTP/2 Announce Support via libcurl #8025
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
base: master
Are you sure you want to change the base?
HTTP/2 Announce Support via libcurl #8025
Conversation
Dedicated Thread vs ASIO integrationAfter prototyping both approaches, I chose a single-threaded libcurl design over direct ASIO integration due to fundamental socket handling incompatibilities that would have required extensive platform-specific code. The Core Problem: Socket Descriptor Impedance MismatchPlatform-Specific Socket Types// Windows: SOCKET (UINT_PTR) vs ASIO's wrapped handles
curl_socket_t sock = ...; // Returns Windows SOCKET
// ASIO expects different wrapper, uses IOCP internally
// Unix: int fd vs ASIO's epoll/kqueue abstractions
int fd = ...; // Raw file descriptor from curl
// ASIO manages its own epoll/kqueue stateEvent Model Incompatibilitylibcurl's // libcurl needs: "Watch this socket for READ only, now WRITE only, now BOTH"
case CURL_POLL_IN: // Register for read events only
case CURL_POLL_OUT: // Register for write events only
case CURL_POLL_REMOVE: // Unregister all events
// ASIO model: async operations own the socket
socket.async_read_some(...); // Can't easily switch to write-only monitoringPlatform Event Systems Don't MixEach platform would need custom bridging code:
Socket Ownership Conflicts// Race condition example:
// ASIO thinks it owns socket → schedules async_read
// libcurl closes socket → curl_easy_cleanup()
// ASIO continues → crash on invalid socketPragmatic SolutionI chose a dedicated thread with
Trade-offs
ConclusionFor tracker announces, the complexity of ASIO integration wasn't justified. The single-threaded design prioritizes correctness, portability, and maintainability over theoretical performance gains that might not materialize in practice. |
82432e3 to
2666ad3
Compare
cc42017 to
b962d2a
Compare
2648005 to
ff1b1a6
Compare
|
The two first commits don't belong to this PR. Are they a prerequisite for this to work? |
arvidn
left a comment
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.
This is a big enough change to be making against the master branch, rather than RC_2_0. Please rebase it.
Also, it seems to be a lot more complex that it would need to be. I'm not done reviewing all of it yet though.
| namespace libtorrent::aux { | ||
|
|
||
| // Mock implementation for testing | ||
| class TORRENT_EXTRA_EXPORT mock_tracker_client { |
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.
This doesn't look like it ought to be part of the production code. I would have thought this would live under tests/. Is there a good reason to put it here?
| announce(req, handler); | ||
| } | ||
|
|
||
| [[nodiscard]] bool can_reuse() const { |
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.
it really looks suspicious to have this feature just for tests. If the real curl_tracker_connection always is reusable, shouldn't the mock tracker connection mimic that behavior?
The production code where this returns true isn't necessarily well tested.
| class tracker_host_counter { | ||
| private: | ||
| std::unordered_map<std::string, int> m_tracker_ref_counts; | ||
| mutable std::mutex m_mutex; |
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.
it doesn't look right that the counters have their own mutex. Shouldn't the counters be protected by the same mutex as the thing they are counting? i.e. the announce queue I assume.
| } | ||
|
|
||
| void curl_thread_manager::wakeup_curl_thread() { | ||
| // Simple delegation to perform_wakeup for backward compatibility |
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.
backwards compatibility with what?
| // FIX: Allow wakeup during shutdown to prevent deadlock | ||
| // The curl thread needs to wake up from curl_multi_poll() to check | ||
| // the shutdown flag and exit cleanly |
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.
this sounds important
| // Wait for thread to be fully initialized with a timeout | ||
| // This prevents deadlock during session initialization when io_context isn't running yet | ||
| { | ||
| std::unique_lock<std::mutex> lock(manager->m_init_mutex); | ||
| // Wait up to 100ms for initialization | ||
| // If io_context isn't running yet, initialization will complete later | ||
| manager->m_init_cv.wait_for(lock, std::chrono::milliseconds(100), [&manager]{ | ||
| return manager->m_init_status != InitStatus::Pending; | ||
| }); | ||
|
|
||
| // Check if initialization failed (only if status changed from Pending) | ||
| if (manager->m_init_status == InitStatus::Failed) { | ||
| // Join the failed thread before throwing | ||
| if (manager->m_curl_thread.joinable()) { | ||
| manager->m_curl_thread.join(); | ||
| } | ||
| throw std::runtime_error("Failed to initialize curl multi handle"); | ||
| } | ||
| // If still pending, that's OK - initialization will complete asynchronously | ||
| } |
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.
this doesn't look right. std::thread will throw an exception if it fails. If it doesn't fail, the thread will be created. It seems quite unnecessary to handle the case where std::thread or the operating system isn't functioning properly.
| } | ||
| } | ||
|
|
||
| std::vector<curl_request> curl_thread_manager::swap_pending_requests() { |
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.
this name does not describe what this function does.
I think what you want is, essentially:
std::unique_lock<std::mutex> lock(m_queue_mutex);
return std::exchange(m_request_queue, std::vector<curl_request>());
But it's not clear why.
| { | ||
| std::scoped_lock<std::mutex> lock(m_init_mutex); | ||
| m_init_status = InitStatus::Success; | ||
| } |
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.
why not just perform the initialization in the original thread instead?
Now you have to add synchronization just so the original thread can wait for the newly created one. I think it would be simpler if you just create the thread after the initialization is done.
and remove m_init_mutex and m_init_cv
| @@ -0,0 +1,198 @@ | |||
| #include "test.hpp" | |||
| #include "libtorrent/aux_/mock_tracker_client.hpp" | |||
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.
you should probably move the mock implementation into this test
| { | ||
| io_context ios; | ||
| settings_pack settings; | ||
| mock_tracker_client client(ios, settings); |
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.
what's the point of these tests? They don't seem to actually test any production code, just the mock object.
arvidn
left a comment
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.
I think it would be easier to review and land this in smaller pieces
| m_client->close(); | ||
| } | ||
| cancel(); | ||
| m_man.remove_request(static_cast<aux::http_tracker_connection const*>(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.
will this make the handler be called? If not, there won't be a way to synchronize the threads, is there?
| , m_upload_rate(peer_connection::upload_channel) | ||
| , m_host_resolver(m_io_context) | ||
| #ifdef TORRENT_USE_LIBCURL | ||
| , m_curl_thread_manager(curl_thread_manager::create(m_io_context, m_settings)) |
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.
how come this isn't a member of tracker_manager instead?
| , m_curl_thread_manager(std::move(curl_mgr)) | ||
| { | ||
| aux::session_settings const& session_sett = m_man.settings(); | ||
| settings_pack sett; |
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.
why are you making a copy of the settings here?
| // testing with self-signed certificates or when using a private | ||
| // certificate authority. The file must contain one or more CA | ||
| // certificates in PEM format. Only applies when using libcurl. | ||
| tracker_ca_certificate, |
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.
Does this only work for curl trackers? If so, that should be documented. If not, this seems like a great independent feature, that can be made as a separate PR.
| namespace libtorrent::aux { | ||
|
|
||
| // Non-template interface for SSL session caching | ||
| // This allows C callbacks to interact with the templated connection pool |
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.
isn't curl managing the connection pool?
| io_context&, | ||
| std::string const& url, | ||
| settings_pack const& settings, | ||
| std::shared_ptr<curl_thread_manager> curl_mgr) |
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.
passing in ownership of this object here creates a circular reference. I think this should be a raw reference
| return scrape_url + "?" + build_tracker_query(req, true); | ||
| } | ||
|
|
||
| std::string curl_tracker_client::build_tracker_query(tracker_request const& req, bool scrape) const |
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.
this looks copy-pasted. it would be better to factor it out into a separate function
| } | ||
|
|
||
|
|
||
| std::string curl_tracker_client::scrape_url_from_announce(std::string const& announce) const |
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.
this also looks like it should be a free function
| // This ensures the worker thread is stopped before any member destruction begins, | ||
| // preventing dangling reference to m_settings | ||
| // C++17 style with if-initializer for cleaner scope management | ||
| if (auto manager = std::move(m_curl_thread_manager); manager) |
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.
| if (auto manager = std::move(m_curl_thread_manager); manager) | |
| if (auto manager = std::move(m_curl_thread_manager)) |
This has the same behavior, right?
| #ifndef TORRENT_DISABLE_LOGGING | ||
| session_log(" shutting down curl thread manager"); | ||
| #endif | ||
| manager->shutdown(); |
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.
session_impl separates initiating shutdown from waiting for shutdown. I believe manager->shutdown() does both, right?
Would you prefer me to go through and address the comments that you've made on this PR, or close this one out and start the process of making smaller PRs to land the main functionality of this and then slowly expand out? Just want to be mindful of your time, and I appreciate you taking a look! |
| // Deleter for unique_ptr with custom cleanup | ||
| struct curl_easy_deleter { | ||
| void operator()(CURL* handle) const noexcept { | ||
| if (handle) curl_easy_cleanup(handle); |
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.
unique_ptr doesn't invoke the deleter for nullptr anyways
|
I'm really excited about this pull request, but noticed progress seems to have stalled. Is there any work happening behind the scenes that isn't visible in the repo? FYI: rTorrent (and its libtorrent) recently adopted libcurl's modern connection reuse practices, which significantly reduces bandwidth usage. It would be fantastic to see similar improvements implemented here. I wish I had the technical skills to contribute directly, but unfortunately that's beyond my current abilities. Still very much hoping to see this move forward! |
Fixes #4334
Introduces native HTTP/2 support for tracker announces through libcurl integration, enabling connection multiplexing and improved performance while maintaining full backward compatibility.
Key Changes
Performance
Build
b2 use-libcurl=onBreaking Changes
None - feature is disabled by default