From bb9b5bf4b841ff88840cd83b78079100120bad59 Mon Sep 17 00:00:00 2001 From: Alden Torres Date: Sat, 1 Apr 2017 12:15:20 -0400 Subject: [PATCH] schedule peer removal only if properly attached and handle special case of attach_peer (#1872) schedule peer removal only if properly attached and handle special case of attach_peer --- src/peer_connection.cpp | 2 +- src/session_impl.cpp | 2 +- src/torrent.cpp | 43 ++++++++++++++++++++++++++++++++--------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index 1068ff071..debeffbed 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -4297,7 +4297,7 @@ namespace libtorrent t->remove_peer(this); // we need to do this here to maintain accurate accounting of number of - // unhoke slots. Ideally the updating of choked state and the + // unchoke slots. Ideally the updating of choked state and the // accounting should be tighter if (!m_choked) { diff --git a/src/session_impl.cpp b/src/session_impl.cpp index 1ca8f4267..4ca42889b 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -979,7 +979,7 @@ namespace aux { // we need to give all the sockets an opportunity to actually have their handlers // called and cancelled before we continue the shutdown. This is a bit - // complicated, if there are no "undead" peers, it's safe tor resume the + // complicated, if there are no "undead" peers, it's safe to resume the // shutdown, but if there are, we have to wait for them to be cleared out // first. In session_impl::on_tick() we check them periodically. If we're // shutting down and we remove the last one, we'll initiate diff --git a/src/torrent.cpp b/src/torrent.cpp index 7580b4f10..ebae4e120 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -5288,12 +5288,36 @@ namespace libtorrent TORRENT_ASSERT(std::count(m_peers_to_disconnect.begin() , m_peers_to_disconnect.end(), p) == 0); - std::weak_ptr weak_t = shared_from_this(); - m_peers_to_disconnect.push_back(p); - m_deferred_disconnect.post(m_ses.get_io_service(), [=]() { - std::shared_ptr t = weak_t.lock(); - if (t) t->on_remove_peers(); - }); + // only schedule the peer for actual removal if in fact + // we can be sure peer_connection will be kept alive until + // the deferred function is called. If a peer_connection + // has not associated torrent, the session_impl object may + // remove it at any time, which may be while the non-owning + // pointer in m_peers_to_disconnect (if added to it) is + // waiting for the deferred function to be called. + // + // one example of this situation is if for example, this + // function is called from the attach_peer path and fail to + // do so because of too many connections. + if (p->associated_torrent().lock().get() == this) + { + std::weak_ptr weak_t = shared_from_this(); + m_peers_to_disconnect.push_back(p); + m_deferred_disconnect.post(m_ses.get_io_service(), [=]() + { + std::shared_ptr t = weak_t.lock(); + if (t) t->on_remove_peers(); + }); + } + else + { + // if the peer was inserted in m_connections but instructed to + // be removed from this torrent, just remove it from it, see + // attach_peer logic. + auto const i = sorted_find(m_connections, p); + if (i != m_connections.end()) + m_connections.erase(i); + } torrent_peer* pp = p->peer_info_struct(); if (ready_for_connections()) @@ -5361,9 +5385,10 @@ namespace libtorrent std::vector peers; m_peers_to_disconnect.swap(peers); - for (peer_connection* p : peers) + for (auto p : peers) { TORRENT_ASSERT(p != nullptr); + TORRENT_ASSERT(p->associated_torrent().lock().get() == this); auto const i = sorted_find(m_connections, p); if (i != m_connections.end()) @@ -6766,7 +6791,7 @@ namespace libtorrent // TODO: 2 if peer is a really good peer, maybe we shouldn't disconnect it // perhaps this logic should be disabled if we have too many idle peers // (with some definition of idle) - if (peer && peer->peer_rank() < p->peer_rank()) + if (peer != nullptr && peer->peer_rank() < p->peer_rank()) { #ifndef TORRENT_DISABLE_LOGGING if (should_log()) @@ -6794,7 +6819,7 @@ namespace libtorrent } #endif p->disconnect(errors::too_many_connections, op_bittorrent); - // we have to do this here because from the peer's point of + // we have to do this here because from the peer's point of view // it wasn't really attached to the torrent, but we do need // to let peer_list know we're removing it remove_peer(p);