RSDK-12896 Fix request counter map pruning #5576
Conversation
viambot
added
the
safe to test
robot/web/web_test.go
Outdated
| // It will take ~1s for pion to register the connection as closed on the server. We | ||
| // can't _only_ assert on conn.PeerConn().ConnectionState() here because that's the | ||
| // client-side. | ||
| time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
🤮 lmk if you have better ideas.
There was a problem hiding this comment.
can you check the peer conn on the web server directly? Like if you passed the id of the peer conn into some kind of webSvc.PCIsClosed function, and have the request counter on the web service check that pc maybe? or a separate function for testing that returns all pcs currently tracked on the request counter?
There was a problem hiding this comment.
Thanks for ideas! I think they could work, but I opted for something a bit more hacky.
can you check the peer conn on the web server directly? Like if you passed the id of the peer conn into some kind of webSvc.PCIsClosed function, and have the request counter on the web service check that pc maybe?
Unfortunately the peer connection ID is different on the client and server-side, so conn.PeerConn() from the client-side doesn't give us much information on what the corresponding connection on the server-side will look like.
or a separate function for testing that returns all pcs currently tracked on the request counter?
That's certainly doable, but the pruning of the inactive connection will only actually happen on the next emission of Request limit exceeded, so I can't wait for the number of currently-tracked PCs to drop to 0 at the point where I currently have testutils.WaitForAssertion.
I opted for a third solution which was to wait for the two "connection closed" logs to be emitted: one from the client-side and one from the server-side. This solution does rely on the messaging and fields remaining consistent for those logs, but at least there's no time.Sleep(3 * time.Second) that's bound to flake at some point.
cheukt
left a comment
cheukt
left a comment
There was a problem hiding this comment.
looks fine to me, but an idea for the test
robot/web/web_test.go
Outdated
| // It will take ~1s for pion to register the connection as closed on the server. We | ||
| // can't _only_ assert on conn.PeerConn().ConnectionState() here because that's the | ||
| // client-side. | ||
| time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
can you check the peer conn on the web server directly? Like if you passed the id of the peer conn into some kind of webSvc.PCIsClosed function, and have the request counter on the web service check that pc maybe? or a separate function for testing that returns all pcs currently tracked on the request counter?
viambot
added
safe to test
4 participants
RSDK-12896
Prunes entries from
rc.requestsPerPCandrc.pcToClientMetadatamaps when a request limit exceeded log is generated and a PC has actually been closed (used to be when 5s had passed according to connection time, which was always""for a closed connection). Adds a test that this pruning works.