Skip to content

Refactor peer management#13

Merged
bedeho merged 17 commits into
JoystreamClassic:developmentfrom
mnaamani:refactor-peer-management
May 26, 2017
Merged

Refactor peer management#13
bedeho merged 17 commits into
JoystreamClassic:developmentfrom
mnaamani:refactor-peer-management

Conversation

@mnaamani

@mnaamani mnaamani commented May 16, 2017

Copy link
Copy Markdown
Contributor
  • Removed PeerPluginAdded and PeerPluginRemoved alerts
  • Removed TorrentPluginAdded and TorrentPluginRemoved alerts
  • Install PeerPlugin on all peers except web seeds
  • Maintain peers in two distinct peer maps to reflect status of underlying connection
  • Fix when conectionRemovedFromSession alert is emitted
  • Use libtorrent::peer_id as templated protocol_session connectionId
  • Fix where validPieceReceived alert is emitted
  • Bug fix (processing on_piece_pass/failed) by cleaning up outstanding call maps

@mnaamani mnaamani changed the title WIP - Refactor peer management Refactor peer management May 18, 2017
@mnaamani mnaamani requested a review from bedeho May 18, 2017 11:18

@bedeho bedeho left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A)
My head might be spinning after a really really long day, but I dont think I can review this with any confidence. You will have to walk me through this.

B)
I feel like I dont understand how the two maps relate to one another, hence the request for more clarity on their characterisations. However, I suspect we are not capturing as much as we can at compile time right now by having two maps. If we add some more structure form our types I think we can get some more safety and clarity for free, but lets see.

Comment thread include/extension/TorrentPlugin.hpp Outdated
// your own peer_plugin. If you want to keep references to it, use weak_ptr.
// NB: All peers are added, while not all are added to _session, see below.
std::map<libtorrent::tcp::endpoint, boost::weak_ptr<PeerPlugin> > _peers;
std::map<PeerPlugin*, boost::weak_ptr<PeerPlugin> > _peerPlugins;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we have two maps, I think characterisations of the map members has to be exact.
An exact constraint gives the precise conditions for membership, i.e. an invariant, which by implication tells you exactly when things are added, and when they are removed.

Comment thread include/extension/TorrentPlugin.hpp Outdated
// Maps endpoint to corresponding peer_plugin. Peers get added to this map after the initial
// bittorrent handshake and if they pass the connection screening, and only if no existing established
// connection exists with the same endpoint.
std::map<libtorrent::tcp::endpoint, boost::weak_ptr<PeerPlugin> > _activePeerPlugins;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Coin::PubKeyHash _finalPkHash;
};

struct TorrentPluginAdded : public libtorrent::torrent_alert {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me as if the TorrentPluginAdded/removed events are also gone now? If so, that is really really nice.


struct TorrentPluginAdded : public libtorrent::torrent_alert {

TorrentPluginAdded(libtorrent::aux::stack_allocator& alloc,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware of this issue

#14

status::TorrentPlugin status;
};

struct TorrentPluginRemoved : public libtorrent::torrent_alert {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware of this issue

#14

@bedeho bedeho merged commit 5a1bf01 into JoystreamClassic:development May 26, 2017
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.

2 participants