WIP: add future on asynchronous methods that trigger alerts (should I continue?)

[joriscarrier]

requires that the lifetime of alerts be guaranteed after session::pop_alerts is called, if they are still referenced.

this pull request allows you to have a future on asynchronous methods that trigger alerts:

torrent_handle:

• add_piece
• read_piece
• post_peer_info
• post_status
• post_download_queue
• post_trackers
• set_metadata
• flush_cache
• force_recheck
• save_resume_data
• post_piece_availability
• prioritize_files
• file_priority
• scrape_tracker
• move_storage
• rename_file

session_handle:

• post_torrent_updates
• post_session_stats
• post_dht_stats
• async_add_torrent
• dht_live_nodes
• dht_sample_infohashes
• dht_direct_request
• remove_torrent

[joriscarrier]

a simple example:

auto f = th.async_flush_cache();
f.wait()

and an other more complex:

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

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

[arvidn]

this looks like a parse failure in CodeQL..

[arvidn]

I would think a much simpler approach would be to add client_data_t to the alerts, and accept an optional client_data_t when making the calls.

That way, at least the alert can be associated with the corresponding call in a simple and cheap way. the client_data_t object could even point to a promise object to be invoked.

Although, having the promise be invoked directly by the main thread would save time in synchronizing the threads. It would be possible to do call the promise in a plugin, but it wouldn't be super elegant

[arvidn]

you need a return nullptr; on lines 100 as well

[arvidn]

this looks like a parse failure in CodeQL..

[arvidn]

this looks a bit suspicious. alert allocated in the heterogeneous queue have their lifetimes tied to that queue. throwing exceptions with pointers into it seems like asking for accessing danging pointers

[arvidn]

a is a non-owning pointer to the alert, and the life-time of the alert is tied to when the user calls pop_alerts(). This seems really risky as the future may be checked after the alert object has been deleted.

[arvidn]

passing the pointer to the alert to the future is risky in the same way as the exception. Making sure that the client gets the future's value before calling pop_alerts() is not trivial and, I would expect, a source of errors.

[glassez]

I would think a much simpler approach would be to add client_data_t to the alerts, and accept an optional client_data_t when making the calls.

Offtopic:
Wouldn't it be a good idea to switch to use std::any instead of client_data_t (in future update)?

[arvidn]

std::any has higher overhead and also requires RTTI, which some people like to disable

[glassez]

Doesn't seem like it actually requires RTTI (except maybe type() method which can be easily avoided from being used). There are several confirmations of it on the Stackoverflow etc.
As for possible overhead I don't believe it is so significant that it could play much role in our scenarios.
But we could get advantages in the form of the ability to pass types by value including managed pointers like unique_ptr/shared_ptr.