Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix absurd Announcement Manager locking #15341

Merged
merged 1 commit into from Feb 6, 2019

Conversation

@DaveTBlake
Copy link
Member

commented Jan 26, 2019

Announcement manager is using the same lock for both adding an event to the queue and when sending each event. This means that it is unable to add anything to the queue while it is sending something. Surely a big mistake, as any delay in sending announcements prevents anything from being added to the announcement queue.

I believe this is what is causing the slow down of party mode initialisation when the Allow remote control from applications on other systems setting is enabled as reported here #15130, but may also improve a number of other performance issues such as filling current playlist (queue) with many songs.

The simple fix is to use separate sections for queue and listener list.

Win64 & OSX64 test builds exist (combined with other PR) on mirrors
http://mirrors.kodi.tv/test-builds/osx/x86_64/kodi-20190126-c49c7fb2-LockingFix-x86_64.dmg

@MilhouseVH could you add to a RPi build please

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

Will do, thanks!

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:LockingFixAnnounce branch from 22befcc to c6bab2a Jan 27, 2019
@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2019

Thanks guys, turns out I'm sicker than I expected.

@MartijnKaijser MartijnKaijser added this to the Leia 18.1-rc1 milestone Jan 27, 2019
@DaveTBlake DaveTBlake force-pushed the DaveTBlake:LockingFixAnnounce branch from c6bab2a to ae67845 Jan 29, 2019
@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Having taken a copy of announcers this exits the lock before calling the announcers as @Paxxi suggested in
#6421. There is just no need to keep it locked.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

That strategy only works with smart pointers. With plain pointers there is no guarantee that the listener still exists at the time announce is called. The copy of the pointer is worth nothing if the object has been destroyed already.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Indeed there is no guarantee that the listener still exists at the time announce is called. I thought the idea of the list copy was to avoid missing some of the existing announcers when looping should the count have changed, rather than worrying about the destroyed ones.

However weak the current approach it seems silly to keep a lock unnecessarily.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

The current code is safe because listeners unregister before they get destroyed. You break this design and make the app likely to crash.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:LockingFixAnnounce branch from ae67845 to 2f77c02 Jan 29, 2019
@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Understood. OK, back to the original change of using separate locks one for queue management and another when sending each event, so any delay in sending announcements does not prevents anything from being added to the announcement queue

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

@FernetMenta are you OK with this as it is - a separate lock for listener list management from sending event?

But since we have lock on changes to m_announcers and listeners unregister before they get destroyed, why do we need to take a copy of the list?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

I had a brief look and don't see any issues. But I don't think this will solve the root cause of a problem. Announcements are supposed to be delivered without much processing. You don't have hooked any database queries into announcements, do you?

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

The user issue that started me looking at this does have some other contributing factors, but there are speed improvements from doing this.

Announcements are supposed to be delivered without much processing.

So do we rely on the fact that they are supposed to do something that is not documented and pray? The real use example has 42 listeners (lots of DirectoryProviders), and it takes some time to loop through them even if they all behave perfectly. Then looking more specifically: Python, AirPlay and TCP server all do announcement synchronously with locks.

Needless preventing events from being added while that happens seems like a bug well worth fixing to me.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

jenkins build this please

Build error is just Android 64 job cancelled, all build is OK

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

Been in Millhouse builds, and included in Win64 and OSX test builds, all reported fine. This is good to go into 18.1

@MartijnKaijser MartijnKaijser merged commit f83635d into xbmc:master Feb 6, 2019
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@DaveTBlake DaveTBlake deleted the DaveTBlake:LockingFixAnnounce branch Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.