MoQ Stat gauge discrepancy corrections.

[akash-a-n]

moves moq_stat's onPublishDone to deliverPublishDoneAndRemove so that abandoned publisher are correctly decremented.

---

This change is 

[akash-a-n]

@akash-a-n made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on afrind, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).

---

moxygen/MoQSession.cpp line 1941 at r1 (raw file):

            onPublishDone,
            pendingPublishDone_->statusCode);
        auto token = cancelSource_.getToken();

When a publisher abandons a connection without sending a PUBLISH_DONE we leak an activePublisher count.
So emitting the stat here accounts for all publisher connections that ended without any double decrement risks.
It may not semantically sound right to use onPublishDone. I am happy to create a new MoQStat callback function if we are particular about that.

[afrind]

If this is urgent we can cherry-pick it into release but would prefer we open an upstream PR.

@afrind reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).

---

moxygen/MoQSession.cpp line 1941 at r1 (raw file):

Previously, akash-a-n wrote…

When a publisher abandons a connection without sending a PUBLISH_DONE we leak an activePublisher count.
So emitting the stat here accounts for all publisher connections that ended without any double decrement risks.
It may not semantically sound right to use onPublishDone. I am happy to create a new MoQStat callback function if we are particular about that.

I see, we always deliver publishDone to the app even if we didn't receive one on the wire. It does feel slightly odd not to bump the counter on arrival, mabye this is ok.

I do think we should get this reviewed upstream, since MoQSession changes are likely to cause merge conflicts.

Also, it would be good to verify the changes here with a test.

[akash-a-n]

I don't think this is urgent. We aren't displaying subActivePublishers_ for the demo are we? @peterchave

@akash-a-n made 2 comments.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on afrind, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).

---

moxygen/MoQSession.cpp line 1941 at r1 (raw file):

Previously, afrind wrote…

I see, we always deliver publishDone to the app even if we didn't receive one on the wire. It does feel slightly odd not to bump the counter on arrival, mabye this is ok.

I do think we should get this reviewed upstream, since MoQSession changes are likely to cause merge conflicts.

Also, it would be good to verify the changes here with a test.

Added a test.

[peterchave]

I don't think this is urgent. We aren't displaying subActivePublishers_ for the demo are we? @peterchave

It was included in the overview dashboard, I have removed it for now.
Once the fixes are in, will overhaul the dashboards and review in detail.

[afrind]

@afrind reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).

---

moxygen/MoQSession.cpp line 1941 at r1 (raw file):

Previously, akash-a-n wrote…

Added a test.

Can you make a version of this PR in the upstream repo? I want to discuss with the other moxygen experts at meta

[akash-a-n]

@akash-a-n made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on afrind, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).

---

moxygen/MoQSession.cpp line 1941 at r1 (raw file):

Previously, afrind wrote…

Can you make a version of this PR in the upstream repo? I want to discuss with the other moxygen experts at meta

@afrind , I have not signed the CLA, would it be useful if I raise the upstream PR?

[akash-a-n]

@akash-a-n made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on afrind, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).

---

moxygen/MoQSession.cpp line 1941 at r1 (raw file):

Previously, akash-a-n wrote…

@afrind , I have not signed the CLA, would it be useful if I raise the upstream PR?

create this anyway:
facebookexperimental#158

[afrind]

I morphed this into this upstream PR facebookexperimental#159