added code to avoid answers / offers race on rust client, #915
added code to avoid answers / offers race on rust client, #915xianshijing-lk wants to merge 1 commit intomainfrom
Conversation
ladvoc
left a comment
ladvoc
left a comment
There was a problem hiding this comment.
LGTM ✅, just a few minor questions/suggestions
| }) | ||
| // Prefer compressed payload when present. | ||
| // If decompression fails, only fall back to plain payload when it is non-empty. | ||
| let payload = if !rpc_request.compressed_payload.is_empty() { |
There was a problem hiding this comment.
question: Was adding this code in this PR intentional?
| let latest_offer_id = self.latest_offer_id.load(Ordering::Acquire); | ||
| let latest_answer_id = self.latest_answer_id.load(Ordering::Acquire); | ||
| if answer_id > 0 && (answer_id < latest_offer_id || answer_id <= latest_answer_id) { | ||
| log::warn!( |
There was a problem hiding this comment.
question: Should this be logged at warn level?
| /// Mapping from SDP mid to track ID, used for track resolution in single PC mode | ||
| mid_to_track_id: Mutex<HashMap<String, String>>, | ||
| /// Offer/answer ordering guards for single-PC churn protection. | ||
| next_offer_id: AtomicU32, |
There was a problem hiding this comment.
nitpick: I don’t have a strong opinion here, but since this adds a bunch of related fields, it might make sense to group them into their own struct (e.g., SDPExchangeState) and include that here instead. This would add no overhead and could help clarify that these fields are conceptually related.
| subscriber_pc, | ||
| single_pc_mode, | ||
| mid_to_track_id: Mutex::new(HashMap::new()), | ||
| next_offer_id: AtomicU32::new(1), |
There was a problem hiding this comment.
If you apply the suggestion from above, you can implement default on the struct and initialize here like this:
sdp_exchange_state: SdpExchangeState {
next_offer_id: AtomicU32::new(1),
..Default::default()
},2 participants
similar t JS solution