Skip to content
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

Add support for passing client_data_t to flush_cache #7543

Conversation

joriscarrier
Copy link
Contributor

Allowing users to pass client_data_t as a parameter to the torrent_handle::flush_cache function.
This enhancement allows users to associate custom data with the flush_cache operation;
this association will be retrieved in cache_flushed_alert by the custom data provided.

@arvidn
Copy link
Owner

arvidn commented Nov 14, 2023

I've been considering doing something like this for a while now, but not just for this alert, but for all alerts that are triggered by the client. Do you think it would make sense to put the user_data_t in the base class of the alerts?

I'm thinking that it could be nice to enable clients to have more sophisticated dispatch of alerts on the client side, e.g. calling a handler function or something.

@arvidn
Copy link
Owner

arvidn commented Nov 14, 2023

either way, unfortunately either of these changes are ABI breaking, so should go into master

@joriscarrier
Copy link
Contributor Author

I've been considering doing something like this for a while now, but not just for this alert, but for all alerts that are triggered by the client. Do you think it would make sense to put the user_data_t in the base class of the alerts?

I'm thinking that it could be nice to enable clients to have more sophisticated dispatch of alerts on the client side, e.g. calling a handler function or something.

In my ideal world, I would have liked to:

  • Ensure that alerts are now stored in shared_ptr (the lifetime of alerts is guaranteed after session::pop_alerts is called, if they are still referenced).
  • Introduce new methods for all calls that manually trigger an alert.

This would allow for the following implementation:

std::future<std::shared_ptr<lt::save_resume_data_alert>> f = th.async_save_resume_data();

try {
    auto alert = f.get();
    std::vector<char> buf = lt::write_torrent_file_buf(alert->params);
} catch (lt::save_resume_data_exception const& e) {
    std::shared_ptr<lt::save_resume_data_failed_alert> alert = e.alert;
}

Without this modification, I had considered using a callback that would be executed in the libtorrent thread (after the alert is placed in the queue, at the same level as the on_alert call on extensions). However, since only extensions have been able to execute code on the libtorrent thread until now, I'm not sure it should be allowed outside of those. In fact, it could be misleading for people who use libtorrent at a high level and who are not aware of the 'a word of caution' in the documentation.

std::mutex m;
std::condition_variable cv;

bool done = false;
th.flush_cache([&cv, &done] (const cache_flushed_alert* alert) {
    // for example: here, you must know that you can't sleep
    done = true;
    cv.notify_one();
});

{
    std::unique_lock lk(m);
    cv.wait(lk, []{ return processed; });
}

What do you think about these options? Do you have any others?

@glassez
Copy link
Contributor

glassez commented Nov 16, 2023

I had considered using a callback that would be executed in the libtorrent thread (after the alert is placed in the queue, at the same level as the on_alert call on extensions).

A low-level interface is something that I also miss so much in libtorrent. So I also have to resort to the trick of using extensions for some purposes.
The ability to set callbacks for various key events that would be executed synchronously (in the libtorrent thread) would be useful.

@joriscarrier
Copy link
Contributor Author

A low-level interface is something that I also miss so much in libtorrent. So I also have to resort to the trick of using extensions for some purposes. The ability to set callbacks for various key events that would be executed synchronously (in the libtorrent thread) would be useful.

here's an example with callbacks: https://github.com/joriscarrier/libtorrent/commits/callback_alert
and here's an example with futures: https://github.com/joriscarrier/libtorrent/commits/future_alert

@joriscarrier
Copy link
Contributor Author

closed for these new pull requests #7557 and #7558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants