feat: Webtorrent support #48
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… fail because given trackers do not have the given info_hash
…Tests still do not work because trackers do not have the given info_hash. FIXME
…s) + updated docs
…t be sending correctly
…is commit also acts as a save point.
Contributor
WalkthroughThis update introduces WebRTC and WebSocket tracker support to the codebase. It adds new modules for WebRTC peer transport and WSS tracker communication, updates dependencies to support these protocols, and extends tracker traits and enums. The README is revised for terminology accuracy and to document new features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WssTracker
participant WebSocketTrackerServer
Client->>WssTracker: new() / get_peers()
WssTracker->>WebSocketTrackerServer: Connect via WebSocket
WssTracker->>WebSocketTrackerServer: Send announce request (JSON with SDP offer)
WebSocketTrackerServer-->>WssTracker: Respond with announce reply (JSON)
WssTracker-->>Client: Return peer list (currently stubbed)
sequenceDiagram
participant Application
participant WebRTCProtocol
participant Peer
Application->>WebRTCProtocol: new()
Application->>WebRTCProtocol: connect_peer(peer, id, info_hash)
WebRTCProtocol->>Peer: (Stub) Create DataChannel, SDP offer/answer (planned)
WebRTCProtocol-->>Application: Return PeerKey
Application->>WebRTCProtocol: send_data/receive_data/close_connection
WebRTCProtocol-->>Peer: (Stub) Handle data transfer (planned)
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
README.md (1)
24-24: Fix terminology inconsistency.The line uses "Webtorrent" (lowercase 't') while line 13 correctly uses "WebTorrent" (uppercase 'T'). Additionally, this should be "WebRTC" not "Webtorrent" for consistency.
-- Webtorrent (WebRTC) peer handshakes/initial connections +- WebTorrent (WebRTC) peer handshakes/initial connectionscrates/libtortillas/src/tracker/mod.rs (1)
145-158: Consider using a constant for the default address.While the unwrap is safe with a hardcoded valid address, it's cleaner to use a constant.
+const DEFAULT_PEER_TRACKER_ADDR: &str = "0.0.0.0:6881"; + impl TrackerRequest { pub fn new(peer_tracker_addr: Option<SocketAddr>) -> TrackerRequest { TrackerRequest { ip: None, port: 6881, uploaded: 0, downloaded: 0, left: None, event: Event::Stopped, peer_tracker_addr: peer_tracker_addr - .unwrap_or(SocketAddr::from_str("0.0.0.0:6881").unwrap()), + .unwrap_or_else(|| SocketAddr::from_str(DEFAULT_PEER_TRACKER_ADDR).expect("Invalid default address")), } } }crates/libtortillas/src/peers/webrtc.rs (1)
68-69: Remove#[allow(unused_variables)]when implementing the methods.This attribute should be temporary and removed once the stub implementations are replaced with actual functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
README.md(2 hunks)crates/libtortillas/Cargo.toml(2 hunks)crates/libtortillas/src/peers/mod.rs(1 hunks)crates/libtortillas/src/peers/utp.rs(1 hunks)crates/libtortillas/src/peers/webrtc.rs(1 hunks)crates/libtortillas/src/tracker/http.rs(2 hunks)crates/libtortillas/src/tracker/mod.rs(2 hunks)crates/libtortillas/src/tracker/udp.rs(1 hunks)crates/libtortillas/src/tracker/wss.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
crates/libtortillas/src/tracker/udp.rs (3)
crates/libtortillas/src/tracker/wss.rs (1)
get_interval(280-282)crates/libtortillas/src/tracker/http.rs (1)
get_interval(61-63)crates/libtortillas/src/tracker/mod.rs (1)
get_interval(118-118)
crates/libtortillas/src/tracker/http.rs (3)
crates/libtortillas/src/tracker/mod.rs (2)
urlencode(70-81)get_interval(118-118)crates/libtortillas/src/tracker/wss.rs (1)
get_interval(280-282)crates/libtortillas/src/tracker/udp.rs (1)
get_interval(495-497)
crates/libtortillas/src/peers/webrtc.rs (2)
crates/libtortillas/src/peers/mod.rs (12)
new(50-68)new(205-215)connect_peer(100-105)id(157-157)id(265-267)info_hash(159-159)info_hash(269-271)send_data(106-106)receive_data(107-111)close_connection(112-112)is_peer_connected(113-113)get_connected_peer(114-114)crates/libtortillas/src/peers/utp.rs (7)
new(26-35)connect_peer(54-108)send_data(198-229)receive_data(110-196)close_connection(240-243)is_peer_connected(245-247)get_connected_peer(231-235)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: build_and_test
🔇 Additional comments (10)
crates/libtortillas/src/peers/utp.rs (1)
17-17: LGTM: Cleaned up unused import.The removal of the
instrumentmacro from the tracing imports is appropriate since it's not used in this file.crates/libtortillas/src/peers/mod.rs (1)
25-25: LGTM: WebRTC module correctly exposed.The addition of the
webrtcmodule aligns with the PR objectives and follows the established pattern of other transport protocol modules.crates/libtortillas/src/tracker/udp.rs (1)
495-497: LGTM: Consistent trait interface implementation.The
get_intervalmethod implementation provides a consistent interface across all tracker types and correctly returns the stored interval value.README.md (3)
13-13: LGTM: Corrected terminology from WSS to WebRTC.The change accurately reflects that this refers to WebRTC peer connections, not WebSocket connections.
19-20: LGTM: Added supported handshake protocols.Accurately documents the currently supported uTP and TCP peer handshake capabilities.
36-42: LGTM: Helpful clarification about WebSocket tracker terminology.The new section clearly explains the naming conventions used in the codebase and provides useful context about the different workflow for WebSocket trackers.
crates/libtortillas/Cargo.toml (2)
7-7: LGTM: Added JSON serialization support.
serde_jsonis appropriate for WebSocket tracker communication that likely uses JSON messaging.
32-42: LGTM: Optional TLS dependencies for certificate handling.The optional TLS-related dependencies provide flexibility for different certificate validation scenarios in WebRTC connections.
crates/libtortillas/src/tracker/http.rs (1)
2-6: LGTM! Good refactoring to consolidate common functionality.The changes properly move shared utilities (
urlencode,TrackerRequest) to the parent module and implement the newget_intervalmethod as required by the updated trait.Also applies to: 61-63
crates/libtortillas/src/tracker/mod.rs (1)
70-81: LGTM! Efficient percent-encoding implementation.Good use of
with_capacityfor performance optimization.
Comment on lines
+25
to
+30
| webrtc = "0.12.0" | ||
| tokio-tungstenite = {version = "0.26.2", features=["native-tls"]} | ||
| rustls = { version = "0.23.26" } | ||
| tokio-rustls = "0.26.2" | ||
| futures = "0.3.31" | ||
| futures-channel = "0.3.31" |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify WebRTC and async dependency usage.
The added dependencies support WebRTC and WebSocket functionality. Ensure all dependencies are actually utilized in the new WebRTC and WSS tracker implementations.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the new dependencies are being used in the codebase
# Expected: All new dependencies should have corresponding usage
echo "Checking usage of new dependencies..."
echo "=== serde_json usage ==="
rg "serde_json" --type rust
echo "=== webrtc usage ==="
rg "webrtc" --type rust
echo "=== tokio-tungstenite usage ==="
rg "tungstenite" --type rust
echo "=== rustls usage ==="
rg "rustls" --type rust
echo "=== futures usage ==="
rg "futures" --type rustLength of output: 2078
🏁 Script executed:
#!/bin/bash
# Verify tokio-rustls and futures-channel usage
echo "=== tokio-rustls usage ==="
rg "tokio_rustls" --type rust || echo "no tokio-rustls usage found"
echo "=== futures::channel usage ==="
rg "use futures::channel" --type rust || echo "no futures::channel usage found"
echo "=== futures_channel crate usage ==="
rg "futures_channel" --type rust || echo "no futures_channel crate usage found"Length of output: 538
Remove Unused Dependencies
The following crates are declared in crates/libtortillas/Cargo.toml but have no references in the codebase and should be removed to avoid unnecessary bloat:
- rustls = { version = "0.23.26" }
- tokio-rustls = "0.26.2"
- futures-channel = "0.3.31"
If TLS support or channel-based workflows are intended later, add the corresponding imports and usage; otherwise, deleting these lines will streamline builds.
🤖 Prompt for AI Agents
In crates/libtortillas/Cargo.toml between lines 25 and 30, remove the
dependencies rustls, tokio-rustls, and futures-channel as they are not used
anywhere in the codebase. This will reduce unnecessary bloat and streamline the
build. Only keep these dependencies if you plan to add TLS support or
channel-based workflows later, in which case ensure to add corresponding imports
and usage in the code.
Comment on lines
+86
to
+116
| async fn stream_peers(&mut self) -> Result<mpsc::Receiver<Vec<Peer>>> { | ||
| let (tx, rx) = mpsc::channel(100); | ||
|
|
||
| // Not *super* cheap clone, but not awful | ||
| let mut tracker = self.clone(); | ||
| // Very cheap clone | ||
| let interval = self.get_interval(); | ||
|
|
||
| let tx = tx.clone(); | ||
| // no pre‑captured interval – always read the latest value | ||
| tokio::spawn(async move { | ||
| loop { | ||
| let peers = tracker.get_peers().await.unwrap(); | ||
| trace!( | ||
| "Successfully made request to get peers: {}", | ||
| peers.last().unwrap() | ||
| ); | ||
|
|
||
| // stop gracefully if the receiver was dropped | ||
| if tx.send(peers).await.is_err() { | ||
| warn!("Receiver dropped – stopping peer stream"); | ||
| break; | ||
| } | ||
|
|
||
| // pick up possibly updated interval (never sleep 0s) | ||
| let delay = interval.max(1); | ||
| sleep(Duration::from_secs(delay as u64)).await; | ||
| } | ||
| }); | ||
| Ok(rx) | ||
| } |
Contributor
There was a problem hiding this comment.
Interval value is captured once and never updated.
The comment at line 95 states "no pre‑captured interval – always read the latest value", but the implementation captures the interval once at line 92 and never updates it. This means interval changes won't be respected.
Apply this diff to fix the interval update issue:
async fn stream_peers(&mut self) -> Result<mpsc::Receiver<Vec<Peer>>> {
let (tx, rx) = mpsc::channel(100);
// Not *super* cheap clone, but not awful
let mut tracker = self.clone();
- // Very cheap clone
- let interval = self.get_interval();
let tx = tx.clone();
- // no pre‑captured interval – always read the latest value
tokio::spawn(async move {
loop {
let peers = tracker.get_peers().await.unwrap();
trace!(
"Successfully made request to get peers: {}",
peers.last().unwrap()
);
// stop gracefully if the receiver was dropped
if tx.send(peers).await.is_err() {
warn!("Receiver dropped – stopping peer stream");
break;
}
- // pick up possibly updated interval (never sleep 0s)
- let delay = interval.max(1);
+ // always read the latest interval value
+ let delay = tracker.get_interval().max(1);
sleep(Duration::from_secs(delay as u64)).await;
}
});
Ok(rx)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn stream_peers(&mut self) -> Result<mpsc::Receiver<Vec<Peer>>> { | |
| let (tx, rx) = mpsc::channel(100); | |
| // Not *super* cheap clone, but not awful | |
| let mut tracker = self.clone(); | |
| // Very cheap clone | |
| let interval = self.get_interval(); | |
| let tx = tx.clone(); | |
| // no pre‑captured interval – always read the latest value | |
| tokio::spawn(async move { | |
| loop { | |
| let peers = tracker.get_peers().await.unwrap(); | |
| trace!( | |
| "Successfully made request to get peers: {}", | |
| peers.last().unwrap() | |
| ); | |
| // stop gracefully if the receiver was dropped | |
| if tx.send(peers).await.is_err() { | |
| warn!("Receiver dropped – stopping peer stream"); | |
| break; | |
| } | |
| // pick up possibly updated interval (never sleep 0s) | |
| let delay = interval.max(1); | |
| sleep(Duration::from_secs(delay as u64)).await; | |
| } | |
| }); | |
| Ok(rx) | |
| } | |
| async fn stream_peers(&mut self) -> Result<mpsc::Receiver<Vec<Peer>>> { | |
| let (tx, rx) = mpsc::channel(100); | |
| // Not *super* cheap clone, but not awful | |
| let mut tracker = self.clone(); | |
| let tx = tx.clone(); | |
| tokio::spawn(async move { | |
| loop { | |
| let peers = tracker.get_peers().await.unwrap(); | |
| trace!( | |
| "Successfully made request to get peers: {}", | |
| peers.last().unwrap() | |
| ); | |
| // stop gracefully if the receiver was dropped | |
| if tx.send(peers).await.is_err() { | |
| warn!("Receiver dropped – stopping peer stream"); | |
| break; | |
| } | |
| // always read the latest interval value | |
| let delay = tracker.get_interval().max(1); | |
| sleep(Duration::from_secs(delay as u64)).await; | |
| } | |
| }); | |
| Ok(rx) | |
| } |
🤖 Prompt for AI Agents
In crates/libtortillas/src/tracker/mod.rs between lines 86 and 116, the interval
value is captured once before spawning the async task and never updated inside
the loop, contrary to the comment. To fix this, remove the captured interval
variable and instead call self.get_interval() inside the loop to fetch the
latest interval value each iteration before sleeping. This ensures the delay
respects any interval changes dynamically.
Comment on lines
+30
to
+68
| // This is AI generated. But it works. | ||
| fn hash_to_byte_string(hex_str: &str) -> String { | ||
| // 1) decode hex → raw bytes | ||
| let bytes = Vec::from_hex(hex_str).expect("invalid hex input"); | ||
|
|
||
| // 2) build the escaped string | ||
| let mut out = String::new(); | ||
| for &b in &bytes { | ||
| match b { | ||
| // common C-style escapes | ||
| 0x00 => out.push_str(r"\0"), | ||
| 0x07 => out.push_str(r"\a"), | ||
| 0x08 => out.push_str(r"\b"), | ||
| 0x09 => out.push_str(r"\t"), | ||
| 0x0A => out.push_str(r"\n"), | ||
| 0x0B => out.push_str(r"\v"), | ||
| 0x0C => out.push_str(r"\f"), | ||
| 0x0D => out.push_str(r"\r"), | ||
|
|
||
| // any other C0 control → \u00XX | ||
| 0x01..=0x06 | 0x0E..=0x1F => { | ||
| out.push_str(&format!(r"\u{:04x}", b)); | ||
| } | ||
|
|
||
| // printable ASCII | ||
| 0x20..=0x7E => { | ||
| out.push(b as char); | ||
| } | ||
|
|
||
| // high-bit set → ISO-8859-1 codepoint | ||
| _ => { | ||
| let ch = char::from_u32(b as u32).expect("byte → char failed"); | ||
| out.push(ch); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| out | ||
| } |
Contributor
There was a problem hiding this comment.
Replace expect with proper error handling to avoid panics.
The function can panic on invalid hex input, which is inappropriate for a library. Additionally, the comment about AI generation should be removed or rephrased professionally.
Apply this diff to handle errors properly:
-// This is AI generated. But it works.
-fn hash_to_byte_string(hex_str: &str) -> String {
- // 1) decode hex → raw bytes
- let bytes = Vec::from_hex(hex_str).expect("invalid hex input");
+/// Converts a hex string to a byte string with C-style escape sequences
+fn hash_to_byte_string(hex_str: &str) -> Result<String, hex::FromHexError> {
+ // Decode hex to raw bytes
+ let bytes = Vec::from_hex(hex_str)?;
// 2) build the escaped string
let mut out = String::new();
for &b in &bytes {
match b {
// common C-style escapes
0x00 => out.push_str(r"\0"),
0x07 => out.push_str(r"\a"),
0x08 => out.push_str(r"\b"),
0x09 => out.push_str(r"\t"),
0x0A => out.push_str(r"\n"),
0x0B => out.push_str(r"\v"),
0x0C => out.push_str(r"\f"),
0x0D => out.push_str(r"\r"),
// any other C0 control → \u00XX
0x01..=0x06 | 0x0E..=0x1F => {
out.push_str(&format!(r"\u{:04x}", b));
}
// printable ASCII
0x20..=0x7E => {
out.push(b as char);
}
// high-bit set → ISO-8859-1 codepoint
_ => {
- let ch = char::from_u32(b as u32).expect("byte → char failed");
+ // For bytes 0x80-0xFF, they map directly to Unicode codepoints U+0080-U+00FF
+ let ch = char::from_u32(b as u32).unwrap();
out.push(ch);
}
}
}
- out
+ Ok(out)
}🤖 Prompt for AI Agents
In crates/libtortillas/src/tracker/mod.rs between lines 30 and 68, the function
hash_to_byte_string uses expect on hex decoding which can cause panics on
invalid input; replace expect with proper error handling by changing the
function to return a Result type and propagate errors instead of panicking.
Also, remove or rephrase the AI generation comment to a professional note or
remove it entirely.
Comment on lines
+67
to
+125
| #[async_trait] | ||
| #[allow(unused_variables)] | ||
| impl TransportProtocol for WebRTCProtocol { | ||
| /// Some helpful information: | ||
| /// <https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelinit> | ||
| /// <https://webrtc.org/getting-started/peer-connections-advanced> | ||
| async fn connect_peer( | ||
| &mut self, | ||
| peer: &mut Peer, | ||
| id: Arc<Hash<20>>, | ||
| info_hash: Arc<InfoHash>, | ||
| ) -> Result<PeerKey, PeerTransportError> { | ||
| // let options = Some(RTCDataChannelInit { | ||
| // ordered: Some(true), | ||
| // ..Default::default() | ||
| // }); | ||
| // let data_channel = self | ||
| // .connection | ||
| // .create_data_channel("data", options) | ||
| // .await | ||
| // .map_err(|e| error!("Failed to create data channel!")) | ||
| // .unwrap(); | ||
|
|
||
| // let offer_options = Some(RTCOfferOptions { | ||
| // voice_activity_detection: false, | ||
| // ice_restart: true, | ||
| // }); | ||
| // | ||
| // let sdp_description = self.connection.create_offer(offer_options).await.unwrap(); | ||
| // self | ||
| // .connection | ||
| // .set_local_description(sdp_description) | ||
| // .await | ||
| // .unwrap(); | ||
| Ok(PeerKey::new(IpAddr::from_str("192.168.1.1").unwrap(), 1234)) | ||
| } | ||
| async fn send_data(&mut self, to: PeerKey, data: Vec<u8>) -> Result<(), PeerTransportError> { | ||
| Ok(()) | ||
| } | ||
| async fn receive_data( | ||
| &mut self, | ||
| info_hash: Arc<InfoHash>, | ||
| id: Arc<Hash<20>>, | ||
| ) -> Result<(PeerKey, Vec<u8>), PeerTransportError> { | ||
| Ok(( | ||
| PeerKey::new(IpAddr::from_str("192.168.1.0").unwrap(), 1234), | ||
| vec![0], | ||
| )) | ||
| } | ||
| fn close_connection(&mut self, peer_key: PeerKey) -> Result<()> { | ||
| Ok(()) | ||
| } | ||
| fn is_peer_connected(&self, peer_key: PeerKey) -> bool { | ||
| true | ||
| } | ||
| async fn get_connected_peer(&self, peer_key: PeerKey) -> Option<Peer> { | ||
| Some(Peer::new(IpAddr::from_str("192.168.1.0").unwrap(), 1234)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete WebRTC transport implementation with stub methods.
The current implementation has several issues:
- All methods return dummy values or do nothing
is_peer_connectedalways returnstrueregardless of actual connection state- Hardcoded IP addresses (192.168.1.1, 192.168.1.0)
- The
peersHashMap is never populated
This appears to be a work-in-progress implementation.
Would you like me to help implement the WebRTC transport protocol methods properly? I can create an issue to track the completion of this implementation.
🤖 Prompt for AI Agents
In crates/libtortillas/src/peers/webrtc.rs from lines 67 to 125, the
WebRTCProtocol implementation currently uses stub methods with hardcoded IPs,
always returns true for connection checks, and does not manage the peers
HashMap. To fix this, replace dummy return values with actual WebRTC connection
logic, properly manage peer states in the peers HashMap, implement real
connection status checks in is_peer_connected, and remove hardcoded IP addresses
by using dynamic peer information. This will complete the WebRTC transport
protocol implementation.
Comment on lines
+276
to
+277
| let res: Vec<Peer> = vec![]; | ||
| Ok(res) |
Contributor
There was a problem hiding this comment.
Tests will panic due to incomplete implementation.
The get_peers method returns an empty vector, but the tests try to access peers[0], which will panic with index out of bounds.
The peer extraction logic needs to be implemented. Would you like me to help implement the logic to parse peer answers from the WebSocket responses?
Also applies to: 285-348
🤖 Prompt for AI Agents
In crates/libtortillas/src/tracker/wss.rs at lines 276-277 and also from
285-348, the get_peers method currently returns an empty vector, causing tests
that access peers[0] to panic due to index out of bounds. To fix this, implement
the logic to parse peer information from the WebSocket responses and populate
the vector with actual Peer instances extracted from the data. Ensure the method
returns a properly filled vector of peers instead of an empty one.
Comment on lines
+138
to
+207
| async fn get_peers(&mut self) -> Result<Vec<Peer>> { | ||
| self.params.event = Event::Started; | ||
| let mut tracker_request_as_json = serde_json::to_string(&self.params).unwrap(); | ||
| trace!("Generated request parameters"); | ||
|
|
||
| // Generate offers | ||
| let numwant = 1; | ||
|
|
||
| let mut offers = vec![]; | ||
| let timestamp = UNIX_EPOCH.elapsed()?.as_secs(); | ||
|
|
||
| // SDP Offer | ||
| // FIXME: Should unicast address actually be 0.0.0.0? All of WebTorrent's offers/answers are | ||
| // 0.0.0.0 or 127.0.0.1 | ||
| // FIXME: media_name port? | ||
| let sdp_offer = SessionDescription { | ||
| version: 0, | ||
| origin: Origin { | ||
| username: "-".to_string(), | ||
| session_id: timestamp, | ||
| session_version: timestamp, | ||
| network_type: "IN".to_string(), | ||
| address_type: "IP4".to_string(), | ||
| unicast_address: "0.0.0.0".to_string(), | ||
| }, | ||
| session_name: "SDP offer from WebTorrent peer".to_string(), | ||
| session_information: None, | ||
| uri: None, | ||
| email_address: None, | ||
| phone_number: None, | ||
| connection_information: None, | ||
| bandwidth: vec![], | ||
| time_descriptions: vec![], | ||
| time_zones: vec![], | ||
| encryption_key: None, | ||
| attributes: vec![], | ||
| media_descriptions: vec![MediaDescription { | ||
| media_name: MediaName { | ||
| media: "application".to_string(), | ||
| port: RangedPort { | ||
| value: 27764, | ||
| range: None, | ||
| }, | ||
| protos: vec!["UDP".to_string(), "DTLS".to_string(), "SCTP".to_string()], | ||
| formats: vec!["webrtc-datachannel".to_string()], | ||
| }, | ||
| media_title: None, | ||
| connection_information: None, | ||
| bandwidth: vec![], | ||
| encryption_key: None, | ||
| attributes: vec![], | ||
| }], | ||
| }; | ||
| let raw_sdp_offer = sdp_offer.marshal(); | ||
|
|
||
| for _i in 0..numwant { | ||
| let offer = WssOfferWrapper::new(raw_sdp_offer.clone()); | ||
| offers.push(offer); | ||
| } | ||
|
|
||
| // {tracker_request_as_json,info_hash:"xyz",peer_id:"abc",action:"announce",numwant:5,offers:{...}} | ||
| tracker_request_as_json.pop(); | ||
| let request = format!( | ||
| "{},\"info_hash\":\"{}\",\"peer_id\":\"{}\",\"action\":\"announce\",\"numwant\":{}, \"offers\": {} }}", | ||
| tracker_request_as_json, | ||
| hash_to_byte_string(&self.info_hash.to_string()), | ||
| hash_to_byte_string(&self.peer_id.to_string()), | ||
| numwant, | ||
| serde_json::to_string(&offers)? | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper JSON serialization instead of string manipulation.
Building JSON by string concatenation (line 199) is error-prone and can lead to invalid JSON. Also, the hash_to_byte_string function needs error handling as noted in the previous file.
Create a proper struct for the announce request:
#[derive(Serialize)]
struct AnnounceRequest {
#[serde(flatten)]
params: TrackerRequest,
info_hash: String,
peer_id: String,
action: String,
numwant: u32,
offers: Vec<WssOfferWrapper>,
}Then use it:
-// {tracker_request_as_json,info_hash:"xyz",peer_id:"abc",action:"announce",numwant:5,offers:{...}}
-tracker_request_as_json.pop();
-let request = format!(
- "{},\"info_hash\":\"{}\",\"peer_id\":\"{}\",\"action\":\"announce\",\"numwant\":{}, \"offers\": {} }}",
- tracker_request_as_json,
- hash_to_byte_string(&self.info_hash.to_string()),
- hash_to_byte_string(&self.peer_id.to_string()),
- numwant,
- serde_json::to_string(&offers)?
-);
+let request = AnnounceRequest {
+ params: self.params.clone(),
+ info_hash: hash_to_byte_string(&self.info_hash.to_string())?,
+ peer_id: hash_to_byte_string(&self.peer_id.to_string())?,
+ action: "announce".to_string(),
+ numwant,
+ offers,
+};
+let request_json = serde_json::to_string(&request)?;🤖 Prompt for AI Agents
In crates/libtortillas/src/tracker/wss.rs between lines 138 and 207, the code
builds JSON for the announce request by manually concatenating strings, which is
error-prone and can produce invalid JSON. To fix this, define a proper struct
(e.g., AnnounceRequest) that includes the existing params flattened plus fields
like info_hash, peer_id, action, numwant, and offers. Then serialize this struct
directly to JSON using serde_json instead of manipulating strings. Also, ensure
to handle any errors from hash_to_byte_string calls properly.
Comment on lines
+213
to
+277
| self | ||
| .write | ||
| .lock() | ||
| .await | ||
| .send(message) | ||
| .await | ||
| .map_err(|e| { | ||
| error!("Error sending message: {e}"); | ||
| }) | ||
| .unwrap(); | ||
| self | ||
| .write | ||
| .lock() | ||
| .await | ||
| .flush() | ||
| .await | ||
| .map_err(|e| { | ||
| error!("Error sending message: {e}"); | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| trace!("Recieving message from tracker"); | ||
|
|
||
| // This section of code is completely and utterly scuffed. self.read.collect() refuses to | ||
| // work, so this is what we're stuck with for now. | ||
| let output = self | ||
| .read | ||
| .lock() | ||
| .await | ||
| .next() | ||
| .await | ||
| .unwrap() | ||
| .unwrap() | ||
| .into_text() | ||
| .unwrap() | ||
| .to_string(); | ||
|
|
||
| trace!("Message recieved: {}", output); | ||
|
|
||
| // Output should look something like this: | ||
| // {"complete":0,"incomplete":0,"action":"announce","interval":120,"info_hash":"myhash"} | ||
| let res_json: Value = serde_json::from_str(&output).unwrap(); | ||
|
|
||
| // Check for "failure_reason" key (response failed) | ||
| let res_json = res_json.as_object().unwrap(); | ||
| if res_json.contains_key("failure reason") { | ||
| panic!("Error: {}", res_json.get("failure reason").unwrap()); | ||
| } | ||
|
|
||
| self.interval = res_json.get("interval").unwrap().as_u64().unwrap() as u32; | ||
|
|
||
| let answers = self | ||
| .read | ||
| .lock() | ||
| .await | ||
| .next() | ||
| .await | ||
| .unwrap() | ||
| .unwrap() | ||
| .into_text() | ||
| .unwrap() | ||
| .to_string(); | ||
|
|
||
| let res: Vec<Peer> = vec![]; | ||
| Ok(res) |
Contributor
There was a problem hiding this comment.
Replace unwrap() calls with proper error propagation.
The method has many unwrap() calls that can panic. Use consistent error handling throughout.
Examples of fixes needed:
-self
- .write
- .lock()
- .await
- .send(message)
- .await
- .map_err(|e| {
- error!("Error sending message: {e}");
- })
- .unwrap();
+self
+ .write
+ .lock()
+ .await
+ .send(message)
+ .await
+ .map_err(|e| {
+ error!("Error sending message: {e}");
+ anyhow::anyhow!("Failed to send message to tracker: {}", e)
+ })?;-let output = self
- .read
- .lock()
- .await
- .next()
- .await
- .unwrap()
- .unwrap()
- .into_text()
- .unwrap()
- .to_string();
+let output = self
+ .read
+ .lock()
+ .await
+ .next()
+ .await
+ .ok_or_else(|| anyhow::anyhow!("No response from tracker"))?
+ .map_err(|e| anyhow::anyhow!("Failed to read tracker response: {}", e))?
+ .into_text()
+ .map_err(|e| anyhow::anyhow!("Invalid text response: {}", e))?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/libtortillas/src/tracker/wss.rs between lines 213 and 277, the code
uses multiple unwrap() calls that can cause panics. Replace all unwrap() calls
with proper error handling by propagating errors using the ? operator or
matching on Result/Option types. Ensure that each asynchronous call and JSON
parsing step returns an appropriate error instead of panicking, maintaining
consistent error handling throughout the method.
Comment on lines
+90
to
+122
| impl WssTracker { | ||
| pub async fn new( | ||
| uri: String, | ||
| info_hash: InfoHash, | ||
| peer_tracker_addr: Option<SocketAddr>, | ||
| ) -> Self { | ||
| let mut peer_id_bytes = [0u8; 20]; | ||
| rand::fill(&mut peer_id_bytes); | ||
| let peer_id = Hash::new(peer_id_bytes); | ||
| debug!(peer_id = %peer_id, "Generated peer ID"); | ||
|
|
||
| trace!("Attemping connection to WSS tracker: {}", uri); | ||
|
|
||
| let (stream, _) = connect_async(&uri) | ||
| .await | ||
| .map_err(|e| { | ||
| error!("Error connecting to peer: {}", e); | ||
| }) | ||
| .unwrap(); | ||
| let (write, read) = stream.split(); | ||
| let arc_write = Arc::new(Mutex::new(write)); | ||
| let arc_read = Arc::new(Mutex::new(read)); | ||
| trace!("Connected to WSS tracker at {}", uri); | ||
|
|
||
| WssTracker { | ||
| info_hash, | ||
| params: TrackerRequest::new(peer_tracker_addr), | ||
| peer_id, | ||
| interval: u32::MAX, | ||
| write: arc_write, | ||
| read: arc_read, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Improve error handling in constructor.
The current error handling logs the error but then panics with unwrap(). This should return a Result instead.
Apply this diff to properly handle errors:
-pub async fn new(
+pub async fn new(
uri: String,
info_hash: InfoHash,
peer_tracker_addr: Option<SocketAddr>,
-) -> Self {
+) -> Result<Self> {
let mut peer_id_bytes = [0u8; 20];
rand::fill(&mut peer_id_bytes);
let peer_id = Hash::new(peer_id_bytes);
debug!(peer_id = %peer_id, "Generated peer ID");
trace!("Attemping connection to WSS tracker: {}", uri);
let (stream, _) = connect_async(&uri)
.await
.map_err(|e| {
error!("Error connecting to peer: {}", e);
- })
- .unwrap();
+ anyhow::anyhow!("Failed to connect to WSS tracker: {}", e)
+ })?;
let (write, read) = stream.split();
let arc_write = Arc::new(Mutex::new(write));
let arc_read = Arc::new(Mutex::new(read));
trace!("Connected to WSS tracker at {}", uri);
- WssTracker {
+ Ok(WssTracker {
info_hash,
params: TrackerRequest::new(peer_tracker_addr),
peer_id,
interval: u32::MAX,
write: arc_write,
read: arc_read,
- }
+ })
}Update the calling code to handle the Result:
-let mut tracker = WssTracker::new(
+let mut tracker = WssTracker::new(
uri.clone(),
info_hash,
Some(SocketAddr::from(([0, 0, 0, 0], port))),
)
-.await;
+.await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl WssTracker { | |
| pub async fn new( | |
| uri: String, | |
| info_hash: InfoHash, | |
| peer_tracker_addr: Option<SocketAddr>, | |
| ) -> Self { | |
| let mut peer_id_bytes = [0u8; 20]; | |
| rand::fill(&mut peer_id_bytes); | |
| let peer_id = Hash::new(peer_id_bytes); | |
| debug!(peer_id = %peer_id, "Generated peer ID"); | |
| trace!("Attemping connection to WSS tracker: {}", uri); | |
| let (stream, _) = connect_async(&uri) | |
| .await | |
| .map_err(|e| { | |
| error!("Error connecting to peer: {}", e); | |
| }) | |
| .unwrap(); | |
| let (write, read) = stream.split(); | |
| let arc_write = Arc::new(Mutex::new(write)); | |
| let arc_read = Arc::new(Mutex::new(read)); | |
| trace!("Connected to WSS tracker at {}", uri); | |
| WssTracker { | |
| info_hash, | |
| params: TrackerRequest::new(peer_tracker_addr), | |
| peer_id, | |
| interval: u32::MAX, | |
| write: arc_write, | |
| read: arc_read, | |
| } | |
| } | |
| impl WssTracker { | |
| pub async fn new( | |
| uri: String, | |
| info_hash: InfoHash, | |
| peer_tracker_addr: Option<SocketAddr>, | |
| ) -> Result<Self> { | |
| let mut peer_id_bytes = [0u8; 20]; | |
| rand::fill(&mut peer_id_bytes); | |
| let peer_id = Hash::new(peer_id_bytes); | |
| debug!(peer_id = %peer_id, "Generated peer ID"); | |
| trace!("Attemping connection to WSS tracker: {}", uri); | |
| let (stream, _) = connect_async(&uri) | |
| .await | |
| .map_err(|e| { | |
| error!("Error connecting to peer: {}", e); | |
| anyhow::anyhow!("Failed to connect to WSS tracker: {}", e) | |
| })?; | |
| let (write, read) = stream.split(); | |
| let arc_write = Arc::new(Mutex::new(write)); | |
| let arc_read = Arc::new(Mutex::new(read)); | |
| trace!("Connected to WSS tracker at {}", uri); | |
| Ok(WssTracker { | |
| info_hash, | |
| params: TrackerRequest::new(peer_tracker_addr), | |
| peer_id, | |
| interval: u32::MAX, | |
| write: arc_write, | |
| read: arc_read, | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/libtortillas/src/tracker/wss.rs from lines 90 to 122, the new()
function currently logs connection errors but then panics by calling unwrap().
Change the function signature to return a Result<Self, Error> instead of Self.
Replace unwrap() with proper error propagation using the ? operator after
mapping the error to a suitable error type. Update all call sites of new() to
handle the Result by matching or using ? to propagate errors further.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the moment, there is no intention of merging this. This PR is made purely to raise "awareness" of the severe lack of documentation of WebTorrent that prohibits us from moving forward with this branch.