Writing a BitTorrent client in JavaScript

Question

I'm trying to write a JavaScript BitTorrent client in Node.js to get more familiar with the language and ecosystem. I don't know how often people use classes in JavaScript since I feel like the overall approach is to try to hide the least state possible and embrace some of the functional programming styles with callbacks and so on. I'd like to know if this use of classes that I came up with is justified and what are other better design ideas.

It is the code for my Tracker Client. It's responsible for taking to Trackers and get the list of peers that have a file. It's not accountable for downloading files.

class TrackerClient {
 constructor(torrent, trackerUrl, trackerPort) {
 this.torrent = torrent;
 this.trackerUrl = trackerUrl;
 this.trackerPort = trackerPort;
 this.clientName = '-VC0001-'; //vinny client version 0001 see: http://www.bittorrent.org/beps/bep_0020.html
 this.socket = null;
 this.transactionId = null;
 this.connectionId = null;
 this.peerId = null;
 }
 _buildConnectRequest() {
 // See: http://www.bittorrent.org/beps/bep_0015.html
 /* build buffer */
 return buffer; 
 }
 _buildAnnounceRequest() {
 // See: http://www.bittorrent.org/beps/bep_0015.html
 /* build buffer */
 return buffer; 
 }
 _initSocket() {
 this.socket = dgram.createSocket('udp4');
 this.socket.on('error', (err) => {
 console.log(`socket error:\n${err.stack}`);
 this.socket.close();
 })
 //ok so this is very weird and i don't even know if you should do it
 this.socket.on('message', this._responseHandler.bind(this));
 }
 _responseHandler(response) {
 console.log('response ', response);
 const type = response.readUInt32BE(0);
 if (type == 0x0) {
 // connect response
 const serverTransactionId = response.readUInt32BE(4);
 if (serverTransactionId != this.transactionId) {
 // throw exception???
 console.log("server transaction id doesn't match with client's");
 console.log('received ', serverTransactionId);
 console.log('got ', this.transactionId);
 }
 // save connection id for later
 this.connectionId = response.readBigUInt64BE(8);
 console.log('connection id ', this.connectionId);
 this._sendAnnounce();
 } else if (type == 0x1) {
 // announce response
 const serverTransactionId = response.readUInt32BE(4);
 if (serverTransactionId != this.transactionId) {
 // throw exception???
 console.log("server transaction id doesn't match with client's");
 console.log('received ', serverTransactionId);
 console.log('got ', this.transactionId);
 }
 const interval = response.readUInt32BE(8);
 const leechers = response.readUInt32BE(12);
 const seeders = response.readUInt32BE(16);
 const peers = {};
 console.log('number of peers ', leechers + seeders);
 // todo: we have to return this through getpeeers()
 for (let i = 20; i < response.length; i += 6) {
 const ip_address = response.slice(i, i + 4).join('.');
 const tcp_port = response.readUInt16BE(i + 4);
 peers[ip_address] = tcp_port;
 console.log('peer ip ', ip_address);
 console.log('peer port ', tcp_port);
 }
 } else {
 // Unknown
 }
 }
 // todo: merge send funcitons
 // We should probably fire a timeout after sending the messages to check later if we got a response
 _sendAnnounce() {
 const request = this._buildAnnounceRequest();
 this.socket.send(request, this.trackerPort, this.trackerUrl);
 }
 _sendConnect() {
 const request = this._buildConnectRequest();
 this.socket.send(request, this.trackerPort, this.trackerUrl);
 }
 getPeers() {
 if (this.socket === null) {
 this._initSocket();
 this._sendConnect();
 }
 // else return from the class attributes?
 }
}

One of my concerns is in this line:

this.socket.on('message', this._responseHandler.bind(this));

I'm binding it because I need to track the state of the connection because that's how the protocol is written. I have to check values from the responses that depend on values that happened in the past. But then, is passing state to a callback like that good design? Also, whenever I get the list of peers, how would I return it to the user? I'm on the response handler callback at that point.

Answer 1

A short review;

• Your implementation of _buildConnectRequest and _buildAnnounceRequest is missing, it would have been interesting to review that

• I am not a big fan of using underscores to denote private methods. If you run version 12 or later of Node, you can prefix with # to denote private functions.

• Your code does not support IPV6 at all. You will have to check the second parameter that on.message provides.

• JsHint is almost perfect, you are missing a semicolon on line 33

• There seems to be no point to declare this.socket = null; You can just write in getPeers() then the following:

  if (!this.socket) {
   this._initSocket();
   this._sendConnect();
  }
  

• Don't call console.log directly, use an intermediary function that takes a severity (so that you can dial down the logging) and that routes logging to either console.log or a log file

• I think this.socket.on('message', this._responseHandler.bind(this)); is fine since this is not set by on.message.

• calvines: Thank you so much for the review! Yesterday I ended up doing a major refactor. Of course I hadn't seen your comment before doing it. I would be very interested in getting your opinion, could I edit the main post or maybe message you the commit url?
• konijn: Definitely don't edit the main post, you can drop the commit URL here. I may ask you to build a new question after that.
• calvines: Sounds good! Here you have @konjin
• konijn: @calvines That code looks fine, I did not find any whoppers there.