[armaan4658]

Description

GetStats() on a closed PeerConnection logged three the agent is closed
errors and lost most of its stats. This keeps the final values, as discussed
in the review.

What gets saved, just before each is torn down:

• ICE candidate pairs and candidates: in ICETransport.stop(), before the agent
  is closed. collectStats then returns the copy, so it no longer queries the
  closed agent and the logs are gone.
• SCTP transport: in SCTPTransport.Stop(), before Abort. Stats() keeps
  returning these values after close.
• Inbound and outbound RTP: in PeerConnection.close(), before the transceivers
  stop. Stopping them removes the stream stats, and a stopped sender reports
  nothing.

Data channel, DTLS, certificate and codec stats were already available after
close.

This does change what a closed PeerConnection reports (more stats instead of
none or zeros), so this PR is now based on next, as requested. Outbound RTP
is new since the last review, because next reports it and main doesn't.

Not covered:

• A sender or receiver stopped before Close(), such as during renegotiation,
  still loses its stats.
• ICETransport.GetSelectedCandidatePairStats() still queries the closed agent.
• A log can still appear if GetStats() is already querying the agent as it
  closes.

Two things I'd like your view on:

• Inbound RTP stats keep frozen counters but show the current Timestamp on each
  call, while outbound RTP, ICE and SCTP keep their closing timestamp.
• RTPReceiver.collectStats reads remoteTrack.codec without the track's lock
  while checkAndUpdateTrack writes it. My test hit this under -race while
  polling GetStats() as the first packet arrived, so the test now waits for
  OnTrack. I left that read unchanged, since it isn't part of this fix. I can
  open a separate issue for it.

Added TestPeerConnection_GetStats_RetainedAfterClose.

Reference issue

Fixes #2768

[JoTurk]

we should also retain the final counters values after shutdown

[codecov]

Codecov Report

❌ Patch coverage is 96.29630% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.58%. Comparing base (8cfe1fb) to head (89e1eef).

Files with missing lines	Patch %	Lines
icegatherer.go	93.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #3678      +/-   ##
==========================================
+ Coverage   91.37%   91.58%   +0.21%     
==========================================
  Files          67       67              
  Lines        7860     7906      +46     
==========================================
+ Hits         7182     7241      +59     
+ Misses        678      665      -13     

Flag	Coverage Δ
go	91.58% <96.29%> (+0.21%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fippo]

we should also retain the final counters values after shutdown

calling getStats on a closed peerconnection returns very, very little these days, basically just the peer-connection report.

[JoTurk]

@fippo but the spec suggests that we keep them?

With a few exceptions, monitored objects, once created, exist for the duration of their associated RTCPeerConnection. This ensures statistics from them are available in the result from getStats() even past the associated peer connection being close

so even if browser violate this currently i think we should follow it since we follow w3c and not browser implementations.

[fippo]

the spec problem is that the transceivers get removed along with .sctp and with them you loose all references to the transport.

[JoTurk]

I thought they were still maintained after shutdown and not removed? where does it say that we should remove them? we currently keep them per the spec https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-close

I also think keeping counters after shutdown will be really helpful for most of our users since they don't have to keep counters at their side. otherwise the fix itself doesn't make much sense.

[fippo]

Given Firefox decade-long failure to not throw on calling pc.getStats() after close we can be reasonably certain nobody cares. I'd align with Chromium/libwebrtc

[JoTurk]

given that the original issue #2768, had

I guess theres another error here... I should still be able to get the candidate pair stats even when ice agent is closed - getting stats/data from a closed connection shouldn't error. I'm assuming the W3C spec allows for this - it should.

then we either make this fix unrelated and to just remove the logs. but doesn't close or fix the issue. or actually retain the counters and fix the original issue

[fippo]

I do not see a point in keeping around ice stats while dropping RTP ones. Lets see what the spec oracles say.

[JoTurk]

Imo we should keep everything after shutdown I can see it being useful for sfus

[armaan4658]

Thanks for the review. Happy to go either way, but I'd like to confirm the scope
before writing more code:

1. Keep this PR to removing the logs, change it to "Relates to Logs written to stdout when accessing stats on a closed connection peer connection #2768", and leave
   the issue open for the stats-retention question.
2. Keep the final stats after close. For ICE that would mean snapshotting the
   candidate pair and candidate stats in ICETransport.stop() before the agent
   is closed, and returning that snapshot from GetStats(). If the goal is to
   keep everything (RTP, transport, SCTP), I think that's a bigger change and
   would probably fit better in its own PR.

Which would you prefer?

[JoTurk]

@armaan4658 feel free to do any or both, no preference from my side, we shouldn't wait for w3c

[armaan4658]

@JoTurk I've updated the PR to keep the final stats after close, and the
description now covers the details. ICE candidate, SCTP and inbound RTP stats
are saved just before teardown, and this also removes the logs. @fippo this
keeps RTP stats along with ICE, so they don't diverge.

Could you take another look? Two questions:

• Are you OK with this on main? It only adds data, but a closed PeerConnection
  now reports more than before.
• RTP stats keep frozen counters but show the current Timestamp on each call,
  while ICE and SCTP keep their closing timestamp. Should I make them consistent?

[JoTurk]

looks good to me, should rebase and merge against next

[armaan4658]

@JoTurk I've rebased this onto next and squashed it into one commit. Could you
approve the workflows when you get a chance? CI hasn't run on the new commit yet.

One addition since your review: next reports outbound RTP stats, so I now keep
those after close too. @fippo that means inbound and outbound RTP are both kept
along with ICE and SCTP.

The rest works the same, but the conflicts meant reworking parts of the ICE and
SCTP code, so the outbound part in rtpsender.go and those two files are the
ones worth another look. The description is updated.

The remaining question is timestamps: inbound RTP shows the current Timestamp
on each call, while outbound RTP, ICE and SCTP keep their closing timestamp.
Should I make inbound match?

[armaan4658]

@JoTurk could you approve the workflows when you get a chance? They're waiting on
approval for the trimmed commit.

[armaan4658]

@JoTurk I've addressed your review and merged the latest next (including the
ICE rc.9 upgrade), with no conflicts. Could you approve the workflows and take a
final look when you have time? Thanks!