Defer processing AvatarAppearance until after first ObjectUpdate#6017
Defer processing AvatarAppearance until after first ObjectUpdate#6017AndrewMeadows wants to merge 6 commits into
Conversation
| S32 mSize; // size of buffer in bytes | ||
| LLHost mHost; // source/dest IP and port | ||
| LLHost mReceivingIF; // source/dest IP and port | ||
| bool mPacketIDChecked = false; |
There was a problem hiding this comment.
We now check the packet sequence ID when we pull it off of the socket. This infrastructure is used to track whether the packet has already been checked or not when it comes time to process it.
| // | ||
| // AvatarAppearance is "Low 158" which means it is stored in four bytes: 0xff 0xff 0x00 0x9E | ||
| // the last two bytes represent 158 in a BigEndian U16 | ||
| return !(header[1] == 255 && header[2] == 0 && header[3] == 158); |
There was a problem hiding this comment.
This is the main fix: we categorize AvatarAppearance messages as low-priority (same as ObjectUpdate) to reduce the race condition where AvatarAppearance is processed too soon.
| // the ObjectUpdate was lost and is being resent). Defer it until the avatar shows up, | ||
| // instead of dropping it. | ||
| LL_WARNS("Messaging") << "avatar_appearance sent for unknown avatar " << uuid << "; deferring" << LL_ENDL; | ||
| LLVOAvatar::addPendingAvatarAppearance(uuid, mesgsys); |
There was a problem hiding this comment.
There are still ways by which AvatarAppearance can arrive before the first ObjectUpdate. Here we queue the AvatarAppearance data, give it an expiry, and process it later (or expire it) when new avatars are created.
| // | ||
| // AvatarAppearance is "Low 158" which means it is stored in four bytes: 0xff 0xff 0x00 0x9E | ||
| // the last two bytes represent 158 in a BigEndian U16 | ||
| return !(header[1] == 255 && header[2] == 0 && header[3] == 158); |
There was a problem hiding this comment.
return header[1] != 255 || header[2] != 0 || header[3] != 158;
| // Skip genuine duplicate resends, same as checkMessages()/logValidMsg() | ||
| // would do further downstream. | ||
| bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0; | ||
| if (!(recv_resent && cdp->isDuplicateResend(recv_packet_id))) |
There was a problem hiding this comment.
if (!recv_resent || !cdp->isDuplicateResend(recv_packet_id))
276a4b0 to
b9e58d2
Compare
| { | ||
| ++timer_iter; | ||
| } | ||
| } |
There was a problem hiding this comment.
I just now realize: we should check expiries first otherwise we could process an expired appearance for avatarp. Meanwhile, the expiry duration I picked (20 seconds) is mostly arbitrary. I know we can see UDP packets get delayed ~10 seconds on the dual-queues. The viewer will re-ask for AvatarAppearance data on a 120 second timer, so I figure the duration should be between those values.
There was a problem hiding this comment.
Meanwhile Henri Beauchamp suggests (via IM): instead of storing the whole AvatarAppearance data just store the agent_id and then when we reach this context just fire off a new avatartexturesrequest.
4 participants
This PR fixes issue #6009 where the high- and low-priority UDP message queue would exacerbate a race condition where the AvatarAppearance message would be processed before the agent was created by its first ObjectUpdate. It also changes where the packet sequence ID is processed to reduce the WARNING logs about out-of-order packets caused by asymmetric priority during packet floods.