-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
WIP: add future on asynchronous methods that trigger alerts (should I continue?) #7558
Changes from all commits
59ddca3
8b1e19a
0a6e2a2
e8c9157
533c0a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,7 @@ | |
~alert_manager(); | ||
|
||
template <class T, typename... Args> | ||
void emplace_alert(Args&&... args) try | ||
const T* emplace_alert(Args&&... args) try | ||
Check notice Code scanning / CodeQL Unused local variable Note
Variable args is not used.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks like a parse failure in CodeQL.. |
||
{ | ||
std::unique_lock<std::recursive_mutex> lock(m_mutex); | ||
|
||
|
@@ -82,13 +82,15 @@ | |
{ | ||
// record that we dropped an alert of this type | ||
m_dropped.set(T::alert_type); | ||
return; | ||
return nullptr; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need a |
||
} | ||
|
||
T& alert = queue.emplace_back<T>( | ||
m_allocations[m_generation], std::forward<Args>(args)...); | ||
|
||
maybe_notify(&alert); | ||
|
||
return &alert; | ||
} | ||
catch (std::bad_alloc const&) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -743,7 +743,7 @@ bool is_downloading_state(int const st) | |
m_ses.close_connection(p); | ||
} | ||
|
||
void torrent::read_piece(piece_index_t const piece) | ||
void torrent::read_piece(piece_index_t const piece, std::shared_ptr<std::promise<const read_piece_alert*>> promise) | ||
{ | ||
error_code ec; | ||
if (m_abort || m_deleted) | ||
|
@@ -761,7 +761,8 @@ bool is_downloading_state(int const st) | |
|
||
if (ec) | ||
{ | ||
m_ses.alerts().emplace_alert<read_piece_alert>(get_handle(), piece, ec); | ||
auto* a = m_ses.alerts().emplace_alert<read_piece_alert>(get_handle(), piece, ec); | ||
if (promise) promise->set_value(a); | ||
return; | ||
} | ||
|
||
|
@@ -775,21 +776,24 @@ bool is_downloading_state(int const st) | |
{ | ||
// this shouldn't actually happen | ||
boost::shared_array<char> buf; | ||
m_ses.alerts().emplace_alert<read_piece_alert>( | ||
auto* a = m_ses.alerts().emplace_alert<read_piece_alert>( | ||
get_handle(), piece, buf, 0); | ||
if (promise) promise->set_value(a); | ||
return; | ||
} | ||
|
||
std::shared_ptr<read_piece_struct> rp = std::make_shared<read_piece_struct>(); | ||
rp->piece_data.reset(new (std::nothrow) char[std::size_t(piece_size)]); | ||
if (!rp->piece_data) | ||
{ | ||
m_ses.alerts().emplace_alert<read_piece_alert>( | ||
auto* a = m_ses.alerts().emplace_alert<read_piece_alert>( | ||
get_handle(), piece, error_code(boost::system::errc::not_enough_memory, generic_category())); | ||
if (promise) promise->set_value(a); | ||
return; | ||
} | ||
rp->blocks_left = blocks_in_piece; | ||
rp->fail = false; | ||
rp->promise = promise; | ||
|
||
disk_job_flags_t flags{}; | ||
auto const read_mode = settings().get_int(settings_pack::disk_io_read_mode); | ||
|
@@ -1215,13 +1219,15 @@ bool is_downloading_state(int const st) | |
int size = m_torrent_file->piece_size(r.piece); | ||
if (rp->fail) | ||
{ | ||
m_ses.alerts().emplace_alert<read_piece_alert>( | ||
const auto* a = m_ses.alerts().emplace_alert<read_piece_alert>( | ||
get_handle(), r.piece, rp->error); | ||
if (rp->promise) rp->promise->set_value(a); | ||
} | ||
else | ||
{ | ||
m_ses.alerts().emplace_alert<read_piece_alert>( | ||
const auto* a = m_ses.alerts().emplace_alert<read_piece_alert>( | ||
get_handle(), r.piece, rp->piece_data, size); | ||
if (rp->promise) rp->promise->set_value(a); | ||
} | ||
} | ||
} | ||
|
@@ -8397,7 +8403,7 @@ namespace { | |
{ | ||
// we need to keep the object alive during this operation | ||
m_ses.disk_thread().async_release_files(m_storage | ||
, std::bind(&torrent::on_cache_flushed, shared_from_this(), false)); | ||
, std::bind(&torrent::on_cache_flushed, shared_from_this(), false, nullptr)); | ||
m_ses.deferred_submit_jobs(); | ||
} | ||
|
||
|
@@ -8684,26 +8690,35 @@ namespace { | |
m_ses.deferred_submit_jobs(); | ||
} | ||
|
||
void torrent::move_storage(std::string const& save_path, move_flags_t const flags) | ||
void torrent::move_storage(std::string const& save_path, move_flags_t const flags, std::shared_ptr<std::promise<const storage_moved_alert*>> promise) | ||
{ | ||
TORRENT_ASSERT(is_single_thread()); | ||
INVARIANT_CHECK; | ||
|
||
if (m_abort) | ||
{ | ||
if (alerts().should_post<storage_moved_failed_alert>()) | ||
alerts().emplace_alert<storage_moved_failed_alert>(get_handle() | ||
if (alerts().should_post<storage_moved_failed_alert>()) { | ||
auto* a = alerts().emplace_alert<storage_moved_failed_alert>(get_handle() | ||
, boost::asio::error::operation_aborted | ||
, "", operation_t::unknown); | ||
|
||
if (promise) promise->set_exception(std::make_exception_ptr(alert_exception(a))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} else { | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_dont_post_exception<storage_moved_failed_alert>())); | ||
} | ||
return; | ||
} | ||
|
||
// if we don't have metadata yet, we don't know anything about the file | ||
// structure and we have to assume we don't have any file. | ||
if (!valid_metadata()) | ||
{ | ||
if (alerts().should_post<storage_moved_alert>()) | ||
alerts().emplace_alert<storage_moved_alert>(get_handle(), save_path, m_save_path); | ||
if (alerts().should_post<storage_moved_alert>()) { | ||
auto* a = alerts().emplace_alert<storage_moved_alert>(get_handle(), save_path, m_save_path); | ||
if (promise) promise->set_value(a); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} else { | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_dont_post_exception<storage_moved_alert>())); | ||
} | ||
#if TORRENT_USE_UNC_PATHS | ||
std::string path = canonicalize_path(save_path); | ||
#else | ||
|
@@ -8722,14 +8737,18 @@ namespace { | |
std::string path = save_path; | ||
#endif | ||
m_ses.disk_thread().async_move_storage(m_storage, std::move(path), flags | ||
, std::bind(&torrent::on_storage_moved, shared_from_this(), _1, _2, _3)); | ||
, std::bind(&torrent::on_storage_moved, shared_from_this(), _1, _2, _3, promise)); | ||
m_moving_storage = true; | ||
m_ses.deferred_submit_jobs(); | ||
} | ||
else | ||
{ | ||
if (alerts().should_post<storage_moved_alert>()) | ||
alerts().emplace_alert<storage_moved_alert>(get_handle(), save_path, m_save_path); | ||
if (alerts().should_post<storage_moved_alert>()) { | ||
auto* a = alerts().emplace_alert<storage_moved_alert>(get_handle(), save_path, m_save_path); | ||
if (promise) promise->set_value(a); | ||
} else { | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_dont_post_exception<storage_moved_alert>())); | ||
} | ||
|
||
#if TORRENT_USE_UNC_PATHS | ||
m_save_path = canonicalize_path(save_path); | ||
|
@@ -8742,26 +8761,34 @@ namespace { | |
} | ||
|
||
void torrent::on_storage_moved(status_t const status, std::string const& path | ||
, storage_error const& error) try | ||
, storage_error const& error, std::shared_ptr<std::promise<const storage_moved_alert*>> promise) try | ||
{ | ||
TORRENT_ASSERT(is_single_thread()); | ||
|
||
m_moving_storage = false; | ||
if (status == status_t::no_error | ||
|| status == status_t::need_full_check) | ||
{ | ||
if (alerts().should_post<storage_moved_alert>()) | ||
alerts().emplace_alert<storage_moved_alert>(get_handle(), path, m_save_path); | ||
if (alerts().should_post<storage_moved_alert>()) { | ||
auto* a = alerts().emplace_alert<storage_moved_alert>(get_handle(), path, m_save_path); | ||
if (promise) promise->set_value(a); | ||
} else { | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_dont_post_exception<storage_moved_alert>())); | ||
} | ||
m_save_path = path; | ||
set_need_save_resume(torrent_handle::if_config_changed); | ||
if (status == status_t::need_full_check) | ||
force_recheck(); | ||
} | ||
else | ||
{ | ||
if (alerts().should_post<storage_moved_failed_alert>()) | ||
alerts().emplace_alert<storage_moved_failed_alert>(get_handle(), error.ec | ||
if (alerts().should_post<storage_moved_failed_alert>()) { | ||
auto* a = alerts().emplace_alert<storage_moved_failed_alert>(get_handle(), error.ec | ||
, resolve_filename(error.file()), error.operation); | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_exception(a))); | ||
} else { | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_dont_post_exception<storage_moved_failed_alert>())); | ||
} | ||
} | ||
} | ||
catch (...) { handle_exception(); } | ||
|
@@ -9462,7 +9489,7 @@ namespace { | |
&& !m_session_paused; | ||
} | ||
|
||
void torrent::flush_cache() | ||
void torrent::flush_cache(std::shared_ptr<std::promise<const cache_flushed_alert*>> promise) | ||
{ | ||
TORRENT_ASSERT(is_single_thread()); | ||
|
||
|
@@ -9473,18 +9500,22 @@ namespace { | |
return; | ||
} | ||
m_ses.disk_thread().async_release_files(m_storage | ||
, std::bind(&torrent::on_cache_flushed, shared_from_this(), true)); | ||
, std::bind(&torrent::on_cache_flushed, shared_from_this(), true, promise)); | ||
m_ses.deferred_submit_jobs(); | ||
} | ||
|
||
void torrent::on_cache_flushed(bool const manually_triggered) try | ||
void torrent::on_cache_flushed(bool const manually_triggered, std::shared_ptr<std::promise<const cache_flushed_alert*>> promise) try | ||
{ | ||
TORRENT_ASSERT(is_single_thread()); | ||
|
||
if (m_ses.is_aborted()) return; | ||
|
||
if (manually_triggered || alerts().should_post<cache_flushed_alert>()) | ||
alerts().emplace_alert<cache_flushed_alert>(get_handle()); | ||
if (manually_triggered || alerts().should_post<cache_flushed_alert>()) { | ||
auto* a = alerts().emplace_alert<cache_flushed_alert>(get_handle()); | ||
if (promise) promise->set_value(a); | ||
} else { | ||
if (promise) promise->set_exception(std::make_exception_ptr(alert_dont_post_exception<cache_flushed_alert>())); | ||
} | ||
} | ||
catch (...) { handle_exception(); } | ||
|
||
|
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 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