Merge pull request #566 from arvidn/choker-fix-1.1

test_optimistic_unchoking sim fix, cleanup and some refactor

include/libtorrent/aux_/session_impl.hpp

@@ -671,6 +671,10 @@ namespace libtorrent
 
 		private:
 
+			// return the settings value for int setting "n", if the value is
+			// negative, return INT_MAX
+			int get_int_setting(int n) const;
+
 			std::vector<torrent*> m_torrent_lists[num_torrent_lists];
 
 			peer_class_pool m_classes;

simulation/libsimulator

@@ -1 +1 @@
-Subproject commit 277389e87cfddd1747da7a8c7287e52ef592d345
+Subproject commit 66356b0ce99cd126c7c99df50609b3050ba44e5d

simulation/setup_swarm.cpp

@@ -46,6 +46,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "settings.hpp"
 #include "setup_swarm.hpp"
 #include "setup_transfer.hpp" // for create_torrent
+#include "utils.hpp"
 
 namespace lt = libtorrent;
 using namespace sim;
@@ -104,11 +105,6 @@ std::string save_path(int swarm_id, int idx)
 	return path;
 }
 
-lt::address addr(char const* str)
-{
-	return lt::address::from_string(str);
-}
-
 void add_extra_peers(lt::session& ses)
 {
 	auto handles = ses.get_torrents();

simulation/setup_swarm.hpp

@@ -95,9 +95,6 @@ lt::torrent_status get_status(lt::session& ses);
 
 std::string save_path(int swarm_id, int idx);
 
-// construct an address from string
-lt::address addr(char const* str);
-
 // disable TCP and enable uTP
 void utp_only(lt::settings_pack& pack);
 

simulation/test_auto_manage.cpp

@@ -38,6 +38,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "settings.hpp"
 #include "create_torrent.hpp"
 #include "simulator/simulator.hpp"
+#include "simulator/utils.hpp"
 #include <iostream>
 
 using namespace sim;
@@ -80,9 +81,8 @@ void run_test(Settings const& sett, Setup const& setup, Test const& test)
 
 	// set up a timer to fire later, to verify everything we expected to happen
 	// happened
-	lt::deadline_timer timer(*ios);
+	sim::timer t(sim, lt::seconds((num_torrents + 1) * 60)
-	timer.expires_from_now(lt::seconds((num_torrents + 1) * 60));
+		, [&](boost::system::error_code const& ec)
-	timer.async_wait([&](boost::system::error_code const& ec)
 	{
 		test(*ses);
 

simulation/test_checking.cpp

@@ -35,6 +35,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/address.hpp"
 #include "libtorrent/torrent_status.hpp"
 #include "simulator/simulator.hpp"
+#include "simulator/utils.hpp"
 
 #include "test.hpp"
 #include "settings.hpp"
@@ -63,9 +64,7 @@ void run_test(Setup const& setup, Test const& test)
 
 	print_alerts(*ses);
 
-	lt::deadline_timer timer(*ios);
+	sim::timer t(sim, lt::seconds(6), [&](boost::system::error_code const& ec)
-	timer.expires_from_now(lt::seconds(6));
-	timer.async_wait([&](lt::error_code const& ec)
 	{
 		test(*ses);
 

simulation/test_ip_filter.cpp

@@ -40,6 +40,8 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/ip_filter.hpp"
 #include "libtorrent/alert_types.hpp"
 #include "simulator/simulator.hpp"
+#include "simulator/utils.hpp"
+#include "utils.hpp" // for print_alerts
 
 using namespace sim;
 
@@ -85,8 +87,6 @@ void run_test(Setup const& setup
 	, HandleAlerts const& on_alert
 	, Test const& test)
 {
-	const lt::time_point start_time = lt::clock_type::now();
-
 	// setup the simulation
 	sim::default_config network_cfg;
 	sim::simulation sim{network_cfg};
@@ -114,22 +114,11 @@ void run_test(Setup const& setup
 	// the alert notification function is called from within libtorrent's
 	// context. It's not OK to talk to libtorrent in there, post it back out and
 	// then ask for alerts.
-	ses->set_alert_notify([&] { ios.post([&] {
+	print_alerts(*ses, [=](lt::session& ses, lt::alert const* a) {
-		std::vector<lt::alert*> alerts;
+		on_alert(ses, a);
-		ses->pop_alerts(&alerts);
+	});
-		// call the user handler
-		for (auto const a : alerts)
-		{
-			printf("%-3d %s\n", int(lt::duration_cast<lt::seconds>(a->timestamp()
-				- start_time).count()), a->message().c_str());
 
-			on_alert(*ses, a);
+	sim::timer t(sim, lt::seconds(60), [&](boost::system::error_code const& ec)
-		}
-	} ); } );
-
-	lt::deadline_timer timer(ios);
-	timer.expires_from_now(lt::seconds(60));
-	timer.async_wait([&](lt::error_code const& ec)
 	{
 		test(*ses, test_peers);
 

simulation/test_optimistic_unchoke.cpp

@@ -35,7 +35,8 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "create_torrent.hpp"
 #include "bittorrent_peer.hpp"
 #include "settings.hpp"
-#include "print_alerts.hpp"
+#include "utils.hpp"
+#include "simulator/utils.hpp"
 
 #include "libtorrent/alert.hpp"
 #include "libtorrent/alert_types.hpp"
@@ -61,7 +62,8 @@ struct choke_state
 TORRENT_TEST(optimistic_unchoke)
 {
 	int const num_nodes = 20;
-	lt::time_duration const test_duration = libtorrent::seconds(1201);
+	lt::time_duration const test_duration
+		= libtorrent::seconds(num_nodes * 90);
 
 	dsl_config network_cfg;
 	sim::simulation sim{network_cfg};
@@ -82,31 +84,24 @@ TORRENT_TEST(optimistic_unchoke)
 
 	session_proxy proxy;
 
-	boost::shared_ptr<lt::session> ses = boost::make_shared<lt::session>(
+	auto ses = std::make_shared<lt::session>(std::ref(pack), std::ref(ios));
-		boost::ref(pack), boost::ref(ios));
 	ses->async_add_torrent(atp);
 
-	std::vector<boost::shared_ptr<sim::asio::io_service> > io_service;
+	std::vector<std::shared_ptr<sim::asio::io_service> > io_service;
-	std::vector<boost::shared_ptr<peer_conn> > peers;
+	std::vector<std::shared_ptr<peer_conn> > peers;
 
-	ses->set_alert_notify([&]() {
+	print_alerts(*ses);
-		// this function is called inside libtorrent and we cannot perform work
-		// immediately in it. We have to notify the outside to pull all the alerts
-		ios.post(boost::bind(&print_alerts, ses.get(), start_time));
-	});
 
-	lt::deadline_timer timer(ios);
+	sim::timer t(sim, lt::seconds(0), [&](boost::system::error_code const& ec)
-	timer.expires_from_now(libtorrent::seconds(2));
-	timer.async_wait([&](error_code const& ec)
 	{
 		for (int i = 0; i < num_nodes; ++i)
 		{
 			// create a new io_service
 			char ep[30];
 			snprintf(ep, sizeof(ep), "50.0.%d.%d", (i + 1) >> 8, (i + 1) & 0xff);
-			io_service.push_back(boost::make_shared<sim::asio::io_service>(
+			io_service.push_back(std::make_shared<sim::asio::io_service>(
-				boost::ref(sim), addr(ep)));
+				std::ref(sim), addr(ep)));
-			peers.push_back(boost::make_shared<peer_conn>(boost::ref(*io_service.back())
+			peers.push_back(std::make_shared<peer_conn>(std::ref(*io_service.back())
 				, [&,i](int msg, char const* bug, int len)
 				{
 					choke_state& cs = peer_choke_state[i];
@@ -136,7 +131,7 @@ TORRENT_TEST(optimistic_unchoke)
 					char const* msg_str[] = {"choke", "unchoke"};
 
 					lt::time_duration d = lt::clock_type::now() - start_time;
-					boost::uint32_t millis = lt::duration_cast<lt::milliseconds>(d).count();
+					std::uint32_t millis = lt::duration_cast<lt::milliseconds>(d).count();
 					printf("\x1b[35m%4d.%03d: [%d] %s (%d ms)\x1b[0m\n"
 						, millis / 1000, millis % 1000, i, msg_str[msg]
 						, int(lt::duration_cast<lt::milliseconds>(cs.unchoke_duration).count()));
@@ -147,28 +142,32 @@ TORRENT_TEST(optimistic_unchoke)
 		}
 	});
 
-	lt::deadline_timer end_timer(ios);
+	sim::timer t2(sim, test_duration, [&](boost::system::error_code const& ec)
-	timer.expires_from_now(test_duration);
-	timer.async_wait([&](error_code const& ec)
 	{
 		for (auto& p : peers)
 		{
 			p->abort();
 		}
+		ses->set_alert_notify([]{});
 		proxy = ses->abort();
 		ses.reset();
 	});
 
 	sim.run();
 
-	boost::int64_t const duration_ms = lt::duration_cast<lt::milliseconds>(test_duration).count();
+	std::int64_t const duration_ms = lt::duration_cast<lt::milliseconds>(test_duration).count();
-	boost::int64_t const average_unchoke_time = duration_ms / num_nodes;
+	std::int64_t const average_unchoke_time = duration_ms / num_nodes;
 	printf("EXPECT: %" PRId64 " ms\n", average_unchoke_time);
-	for (auto const& cs : peer_choke_state)
+	for (auto& cs : peer_choke_state)
 	{
-		boost::int64_t unchoke_duration = lt::duration_cast<lt::milliseconds>(cs.unchoke_duration).count();
+		if (!cs.choked)
+		{
+			cs.choked = true;
+			cs.unchoke_duration += lt::clock_type::now() - cs.last_unchoke;
+		}
+		std::int64_t const unchoke_duration = lt::duration_cast<lt::milliseconds>(cs.unchoke_duration).count();
 		printf("%" PRId64 " ms\n", unchoke_duration);
-		TEST_CHECK(std::abs(unchoke_duration - average_unchoke_time) < 1000);
+		TEST_CHECK(std::abs(unchoke_duration - average_unchoke_time) < 1500);
 	}
 }
 

simulation/test_swarm.cpp

@@ -32,6 +32,7 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include "setup_swarm.hpp"
 #include "test.hpp"
+#include "utils.hpp"
 #include "libtorrent/alert.hpp"
 #include "libtorrent/alert_types.hpp"
 #include "libtorrent/session.hpp"

simulation/test_tracker.cpp

@@ -35,6 +35,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "setup_swarm.hpp"
 #include "simulator/simulator.hpp"
 #include "simulator/http_server.hpp"
+#include "simulator/utils.hpp"
 #include "libtorrent/alert_types.hpp"
 #include "libtorrent/announce_entry.hpp"
 #include "libtorrent/session.hpp"
@@ -323,7 +324,6 @@ TORRENT_TEST(ipv6_support)
 
 		ses->set_alert_notify(std::bind(&on_alert_notify, ses.get()));
 
-
 		lt::add_torrent_params p;
 		p.name = "test-torrent";
 		p.save_path = ".";
@@ -334,9 +334,8 @@ TORRENT_TEST(ipv6_support)
 		ses->async_add_torrent(p);
 
 		// stop the torrent 5 seconds in
-		asio::high_resolution_timer stop(ios);
+		sim::timer t1(sim, lt::seconds(5)
-		stop.expires_from_now(chrono::seconds(5));
+			, [&ses](boost::system::error_code const& ec)
-		stop.async_wait([&ses](boost::system::error_code const& ec)
 		{
 			std::vector<lt::torrent_handle> torrents = ses->get_torrents();
 			for (auto const& t : torrents)
@@ -346,9 +345,8 @@ TORRENT_TEST(ipv6_support)
 		});
 
 		// then shut down 10 seconds in
-		asio::high_resolution_timer terminate(ios);
+		sim::timer t2(sim, lt::seconds(10)
-		terminate.expires_from_now(chrono::seconds(10));
+			, [&ses,&zombie](boost::system::error_code const& ec)
-		terminate.async_wait([&ses,&zombie](boost::system::error_code const& ec)
 		{
 			zombie = ses->abort();
 			ses->set_alert_notify([]{});
@@ -403,9 +401,8 @@ void tracker_test(Setup setup, Announce a, Test1 test1, Test2 test2
 	ses->async_add_torrent(p);
 
 	// run the test 5 seconds in
-	asio::high_resolution_timer t1(ios);
+	sim::timer t1(sim, lt::seconds(5)
-	t1.expires_from_now(chrono::seconds(5));
+		, [&ses,&test1](boost::system::error_code const& ec)
-	t1.async_wait([&ses,&test1](boost::system::error_code const& ec)
 	{
 		std::vector<lt::torrent_handle> torrents = ses->get_torrents();
 		TEST_EQUAL(torrents.size(), 1);
@@ -413,9 +410,8 @@ void tracker_test(Setup setup, Announce a, Test1 test1, Test2 test2
 		test1(h);
 	});
 
-	asio::high_resolution_timer t2(ios);
+	sim::timer t2(sim, lt::seconds(5 + delay)
-	t2.expires_from_now(chrono::seconds(5 + delay));
+	, [&ses,&test2](boost::system::error_code const& ec)
-	t2.async_wait([&ses,&test2](boost::system::error_code const& ec)
 	{
 		std::vector<lt::torrent_handle> torrents = ses->get_torrents();
 		TEST_EQUAL(torrents.size(), 1);
@@ -424,9 +420,8 @@ void tracker_test(Setup setup, Announce a, Test1 test1, Test2 test2
 	});
 
 	// then shut down 10 seconds in
-	asio::high_resolution_timer t3(ios);
+	sim::timer t3(sim, lt::seconds(10 + delay)
-	t3.expires_from_now(chrono::seconds(10 + delay));
+		, [&ses,&zombie](boost::system::error_code const& ec)
-	t3.async_wait([&ses,&zombie](boost::system::error_code const& ec)
 	{
 		zombie = ses->abort();
 		ses->set_alert_notify([]{});

simulation/test_transfer.cpp

@@ -44,6 +44,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/settings_pack.hpp"
 #include "simulator/simulator.hpp"
 #include "simulator/socks_server.hpp"
+#include "simulator/utils.hpp"
 #include "setup_swarm.hpp"
 #include "utils.hpp"
 
@@ -128,9 +129,7 @@ void run_test(
 	params.save_path = save_path(1);
 	ses[1]->async_add_torrent(params);
 
-	lt::deadline_timer timer(ios0);
+	sim::timer t(sim, lt::seconds(60), [&](boost::system::error_code const& ec)
-	timer.expires_from_now(lt::seconds(60));
-	timer.async_wait([&](lt::error_code const& ec)
 	{
 		test(ses);
 

simulation/utils.cpp

@@ -116,6 +116,11 @@ void set_proxy(lt::session& ses, int proxy_type, int flags, bool proxy_peer_conn
 	ses.apply_settings(p);
 }
 
+lt::address addr(char const* str)
+{
+	return lt::address::from_string(str);
+}
+
 void print_alerts(lt::session& ses
 	, std::function<void(lt::session&, lt::alert const*)> on_alert)
 {

simulation/utils.hpp

@@ -31,6 +31,7 @@ POSSIBILITY OF SUCH DAMAGE.
 */
 
 #include <functional>
+#include "libtorrent/address.hpp"
 
 namespace libtorrent
 {
@@ -40,6 +41,9 @@ namespace libtorrent
 
 namespace lt = libtorrent;
 
+// construct an address from string
+lt::address addr(char const* str);
+
 void utp_only(lt::session& ses);
 void enable_enc(lt::session& ses);
 void filter_ips(lt::session& ses);

src/session_impl.cpp

@@ -3773,6 +3773,13 @@ retry:
 		}
 	}
 
+	int session_impl::get_int_setting(int n) const
+	{
+		int const v = settings().get_int(n);
+		if (v < 0) return (std::numeric_limits<int>::max)();
+		return v;
+	}
+
 	void session_impl::recalculate_auto_managed_torrents()
 	{
 		INVARIANT_CHECK;
@@ -3793,28 +3800,13 @@ retry:
 
 		// these counters are set to the number of torrents
 		// of each kind we're allowed to have active
-		int downloading_limit = settings().get_int(settings_pack::active_downloads);
+		int downloading_limit = get_int_setting(settings_pack::active_downloads);
-		int seeding_limit = settings().get_int(settings_pack::active_seeds);
+		int seeding_limit = get_int_setting(settings_pack::active_seeds);
-		int checking_limit = settings().get_int(settings_pack::active_checking);
+		int checking_limit = get_int_setting(settings_pack::active_checking);
-		int dht_limit = settings().get_int(settings_pack::active_dht_limit);
+		int dht_limit = get_int_setting(settings_pack::active_dht_limit);
-		int tracker_limit = settings().get_int(settings_pack::active_tracker_limit);
+		int tracker_limit = get_int_setting(settings_pack::active_tracker_limit);
-		int lsd_limit = settings().get_int(settings_pack::active_lsd_limit);
+		int lsd_limit = get_int_setting(settings_pack::active_lsd_limit);
-		int hard_limit = settings().get_int(settings_pack::active_limit);
+		int hard_limit = get_int_setting(settings_pack::active_limit);
-
-		if (downloading_limit == -1)
-			downloading_limit = (std::numeric_limits<int>::max)();
-		if (seeding_limit == -1)
-			seeding_limit = (std::numeric_limits<int>::max)();
-		if (checking_limit == -1)
-			checking_limit = (std::numeric_limits<int>::max)();
-		if (hard_limit == -1)
-			hard_limit = (std::numeric_limits<int>::max)();
-		if (dht_limit == -1)
-			dht_limit = (std::numeric_limits<int>::max)();
-		if (lsd_limit == -1)
-			lsd_limit = (std::numeric_limits<int>::max)();
-		if (tracker_limit == -1)
-			tracker_limit = (std::numeric_limits<int>::max)();
 
 		// if hard_limit is <= 0, all torrents in these lists should be paused.
 		// The order is not relevant
@@ -3882,7 +3874,9 @@ retry:
 		// that are eligible for optimistic unchoke, similar to the torrents
 		// perhaps this could even iterate over the pool allocators of
 		// torrent_peer objects. It could probably be done in a single pass and
-		// collect the n best candidates
+		// collect the n best candidates. maybe just a queue of peers would make
+		// even more sense, just pick the next peer in the queue for unchoking. It
+		// would be O(1).
 		for (connection_map::iterator i = m_connections.begin()
 			, end(m_connections.end()); i != end; ++i)
 		{
@@ -4171,8 +4165,8 @@ retry:
 		TORRENT_ASSERT(is_single_thread());
 		INVARIANT_CHECK;
 
-		time_point now = aux::time_now();
+		time_point const now = aux::time_now();
-		time_duration unchoke_interval = now - m_last_choke;
+		time_duration const unchoke_interval = now - m_last_choke;
 		m_last_choke = now;
 
 		// build list of all peers that are
@@ -4186,8 +4180,8 @@ retry:
 			boost::shared_ptr<peer_connection> p = *i;
 			TORRENT_ASSERT(p);
 			++i;
-			torrent* t = p->associated_torrent().lock().get();
+			torrent* const t = p->associated_torrent().lock().get();
-			torrent_peer* pi = p->peer_info_struct();
+			torrent_peer* const pi = p->peer_info_struct();
 
 			if (p->ignore_unchoke_slots() || t == 0 || pi == 0
 				|| pi->web_seed || t->is_paused())
@@ -4257,8 +4251,11 @@ retry:
 			, allowed_upload_slots);
 #endif
 
-		int num_opt_unchoke = m_settings.get_int(settings_pack::num_optimistic_unchoke_slots);
+		int const unchoked_counter_optimistic
-		if (num_opt_unchoke == 0) num_opt_unchoke = (std::max)(1, allowed_upload_slots / 5);
+			= m_stats_counters[counters::num_peers_up_unchoked_optimistic];
+		int const num_opt_unchoke = (unchoked_counter_optimistic == 0)
+			? (std::max)(1, allowed_upload_slots / 5) : unchoked_counter_optimistic;
+
 		int unchoke_set_size = allowed_upload_slots - num_opt_unchoke;
 
 		// go through all the peers and unchoke the first ones and choke
@@ -6190,10 +6187,7 @@ retry:
 
 	void session_impl::update_unchoke_limit()
 	{
-		int allowed_upload_slots = m_settings.get_int(settings_pack::unchoke_slots_limit);
+		int const allowed_upload_slots = get_int_setting(settings_pack::unchoke_slots_limit);
-
-		if (allowed_upload_slots < 0)
-			allowed_upload_slots = (std::numeric_limits<int>::max)();
 
 		m_stats_counters.set_value(counters::num_unchoke_slots
 			, allowed_upload_slots);
@@ -7144,9 +7138,9 @@ retry:
 				settings_pack::num_optimistic_unchoke_slots));
 		}
 
-		int unchoked_counter_all = m_stats_counters[counters::num_peers_up_unchoked_all];
+		int const unchoked_counter_all = m_stats_counters[counters::num_peers_up_unchoked_all];
-		int unchoked_counter = m_stats_counters[counters::num_peers_up_unchoked];
+		int const unchoked_counter = m_stats_counters[counters::num_peers_up_unchoked];
-		int unchoked_counter_optimistic
+		int const unchoked_counter_optimistic
 			= m_stats_counters[counters::num_peers_up_unchoked_optimistic];
 
 		TORRENT_ASSERT_VAL(unchoked_counter_all == unchokes_all, unchokes_all);