Make Joystream protocole_session compatible with streaming#7
Conversation
bedeho
left a comment
bedeho
left a comment
There was a problem hiding this comment.
As you see, bigger changes may be needed, but only do the minor ones now, I created a new issue which can be tackled later, or if we start running into buggy performance.
| void startDownloading(const Coin::Transaction & contractTx, | ||
| const PeerToStartDownloadInformationMap<ConnectionIdType> & peerToStartDownloadInformationMap); | ||
| const PeerToStartDownloadInformationMap<ConnectionIdType> & peerToStartDownloadInformationMap, | ||
| const std::function<int(const std::vector<detail::Piece<ConnectionIdType>>*)> & pickNextPieceMethod); |
There was a problem hiding this comment.
It will help readability, and ease of future upgrades, to typedef the type for the piecePicker method, rather than having to write out the full type each time. Have a look in callback.hpp, it gives similar examples for other callbacks we have.
|
|
||
| assert(_observing == nullptr && _buying != nullptr && _selling == nullptr); | ||
|
|
||
| if (pickNextPieceMethod != nullptr) { |
There was a problem hiding this comment.
You don´t need this test pickNextPieceMethod != nullptr, since the variable is not a pointer, but a reference. I am surprised the compiler did not, at the very least, warn or complain about this.
|
|
||
| // We did not find anything | ||
| throw std::runtime_error("Unable to find any unassigned pieces."); | ||
| assert(this->_pickNextPieceMethod != nullptr); |
There was a problem hiding this comment.
This assert is also not needed, see point above about referencs.
|
|
||
| uint32_t i = this->_pickNextPieceMethod(&_pieces); | ||
|
|
||
| std::cout << "Hello next piece : " << i << std::endl; |
There was a problem hiding this comment.
As you mentioned, lets drop all logging to std::cout. If anything, log to std::clog, but lets avoid even that for now, as we dont have a well planned logging infrastructure on the c++ side.
| throw std::runtime_error("Unable to find any unassigned pieces."); | ||
| assert(this->_pickNextPieceMethod != nullptr); | ||
|
|
||
| uint32_t i = this->_pickNextPieceMethod(&_pieces); |
There was a problem hiding this comment.
If you just return uint32_t, then there is not way for the method to tell you that no piece was available, which can certainly happen at times. You are better off introducing a new exception in this library, e.g. called NoPieceAvailableException, and allow the callback to throw it. Combine this with entirely dropping this routine (getNextUnassignedPiece), and just have the caller of this method directly call _pickNextPieceMethod, and catch if NoPieceAvailableException is thrown. Currently, it is catching std::runtime_error for the same purpose.
| void Session<ConnectionIdType>::startDownloading(const Coin::Transaction & contractTx, const PeerToStartDownloadInformationMap<ConnectionIdType> & peerToStartDownloadInformationMap) { | ||
| void Session<ConnectionIdType>::startDownloading(const Coin::Transaction & contractTx, | ||
| const PeerToStartDownloadInformationMap<ConnectionIdType> & peerToStartDownloadInformationMap, | ||
| const std::function<int(const std::vector<detail::Piece<ConnectionIdType>>*)> & pickNextPieceMethod) { |
There was a problem hiding this comment.
There are two problems with this callback type
-
We are giving the user a raw pointer to your piece vector, which allows the user to perhaps change or corrupt the information.
-
Even if the user could not change the information, e.g. if we passed a
const *, then we are still telling the user about too much information which is not of relevance, just look at how much information is in thedetail::Piececlass, and also about pieces which are no longer relevant. The only thing the callback really needs to know is which piece indexes it is allowed to pick from each time. -
We are exposing the
detail::Piecetype, which is supposed to be private, at least since its in thedetailnamespace.
I think a proper fix to these issues will require a larger rewrite of protocol_session, so for now, perhaps just fixing 1) is enough. I will add an issue about this on this repo. If you finish the other work, we can circle back to this later.
See #8
2 participants
Will pick next piece to buy by priority.