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

[PVR] Make PVR database threadsafe. #12844

Merged
merged 4 commits into from Sep 30, 2017

Conversation

Projects
None yet
3 participants
@ksooo
Member

ksooo commented Sep 27, 2017

class CPVRDatabase had no concurrency protection features but is accessed simultaneously from multiple threads. :-/ This PR introduces basic concurrency support for CPVRDatabase.

Example:

Thread 43 (Thread 0x59d65390 (LWP 736)):
#0  0x007480f8 in dbiplus::field_prop* std::__uninitialized_default_n_1::__uninit_default_n(dbiplus::field_prop*, unsigned int) ()
#1  0x00748164 in std::vector >::_M_default_append(unsigned int) ()
#2  0x00747774 in dbiplus::SqliteDataset::query(std::string const&) ()
#3  0x0073ddd4 in CDatabase::GetSingleValue(std::string const&, std::unique_ptr >&) ()
#4  0x0073ee90 in CDatabase::GetSingleValue(std::string const&, std::string const&, std::string const&, std::string const&) ()
#5  0x008c507c in PVR::CPVRDatabase::PersistChannels(PVR::CPVRChannelGroup&) ()
#6  0x008c696c in PVR::CPVRDatabase::Persist(PVR::CPVRChannelGroup&) ()
#7  0x008aa28c in PVR::CPVRChannelGroup::Persist() ()
#8  0x008ac814 in PVR::CPVRChannelGroup::UpdateGroupEntries(PVR::CPVRChannelGroup const&) ()
#9  0x008af3bc in PVR::CPVRChannelGroupInternal::UpdateGroupEntries(PVR::CPVRChannelGroup const&) ()
#10 0x008aef20 in PVR::CPVRChannelGroupInternal::Update() ()
#11 0x008b1874 in PVR::CPVRChannelGroups::Update(bool) ()
#12 0x008b24e4 in PVR::CPVRChannelGroupsContainer::Update(bool) ()
#13 0x008d184c in PVR::CPVRChannelGroupsUpdateJob::DoWork() ()
#14 0x008c1370 in PVR::CPVRManagerJobQueue::ExecutePendingJobs() ()
#15 0x008c200c in PVR::CPVRManager::Process() ()
#16 0x0066c9a4 in CThread::Action() ()
#17 0x0066d0a0 in CThread::staticThread(void*) ()
#18 0x76efdd34 in start_thread (arg=0x59d65390) at pthread_create.c:465
#19 0x74fd0ab8 in ?? () at ../sysdeps/unix/sysv/linux/arm/clone.S:73 from /usr/lib/libc.so.6
Backtrace stopped: previous frame identical to this frame (corrupt stack?)


Thread 1 (Thread 0x6e8ff390 (LWP 685)):
#0  0x7519ea60 in std::string::assign(std::string const&) () from /usr/lib/libstdc++.so.6
#1  0x00746978 in dbiplus::SqliteDataset::make_query(std::list >&) ()
#2  0x00745a88 in dbiplus::SqliteDataset::make_insert() ()
#3  0x0073e058 in CDatabase::CommitInsertQueries() ()
#4  0x008a7a78 in PVR::CPVRChannel::Persist() ()
#5  0x008ad160 in PVR::CPVRChannelGroup::SearchAndSetChannelIcons(bool) ()
#6  0x008b264c in PVR::CPVRChannelGroupsContainer::SearchMissingChannelIcons() const ()
#7  0x008d1474 in PVR::CPVRSearchMissingChannelIconsJob::DoWork() ()
#8  0x00627984 in CJobWorker::Process() ()
#9  0x0066c9a4 in CThread::Action() ()
#10 0x0066d0a0 in CThread::staticThread(void*) ()
#11 0x76efdd34 in start_thread (arg=0x6e8ff390) at pthread_create.c:465
#12 0x74fd0ab8 in ?? () at ../sysdeps/unix/sysv/linux/arm/clone.S:73 from /usr/lib/libc.so.6
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

This should fix several occasional crashes reported in the forum and elsewhere.

Example: https://forum.kodi.tv/showthread.php?tid=298461&pid=2651405#pid2651405 (second crashlog in that posting)

I runtime-tested this change on macOS, latest Kodi master.

@Jalle19 for review?

@ksooo ksooo added this to the L 18.0-alpha1 milestone Sep 27, 2017

@ksooo ksooo requested a review from Jalle19 Sep 27, 2017

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Sep 27, 2017

Member

I've thought about this too, but I was somehow under the impression that the database itself can handle being called from multiple threads just fine? Do we hae locking like this in VideoDatabase too?

Member

Jalle19 commented Sep 27, 2017

I've thought about this too, but I was somehow under the impression that the database itself can handle being called from multiple threads just fine? Do we hae locking like this in VideoDatabase too?

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Sep 27, 2017

Member

AFAIK, no.

Member

ksooo commented Sep 27, 2017

AFAIK, no.

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Sep 27, 2017

Member

@MilhouseVH could you include this PR in your next build? I would like to get some feedback.

Member

ksooo commented Sep 27, 2017

@MilhouseVH could you include this PR in your next build? I would like to get some feedback.

@a1rwulf

This comment has been minimized.

Show comment
Hide comment
@a1rwulf

a1rwulf Sep 27, 2017

Member

I think the VideoDatabase code does not share database instances across threads, at least the parts I've seen. Sqlite should be compiled with multithread support so also this shouldn't be an issue either.

As far as I can see PVRManager holds the DB connection that is shared, so this changes are very needed.

Member

a1rwulf commented Sep 27, 2017

I think the VideoDatabase code does not share database instances across threads, at least the parts I've seen. Sqlite should be compiled with multithread support so also this shouldn't be an issue either.

As far as I can see PVRManager holds the DB connection that is shared, so this changes are very needed.

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Sep 27, 2017

Member

As far as I can see PVRManager holds the DB connection that is shared, so this changes are very needed.

Yep. I have no idea why we have not spotted this earlier. We're getting crash reports containing various pvr database constellations for ages. Personally, I never encountered those crashes and 'always' had better things to do, that's why I have not investigated earlier.

Member

ksooo commented Sep 27, 2017

As far as I can see PVRManager holds the DB connection that is shared, so this changes are very needed.

Yep. I have no idea why we have not spotted this earlier. We're getting crash reports containing various pvr database constellations for ages. Personally, I never encountered those crashes and 'always' had better things to do, that's why I have not investigated earlier.

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo
Member

ksooo commented Sep 29, 2017

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Sep 30, 2017

Member

jenkins build this please

Member

ksooo commented Sep 30, 2017

jenkins build this please

@ksooo

This comment has been minimized.

Show comment
Hide comment
@ksooo

ksooo Sep 30, 2017

Member

jenkins errors are unrelated

Member

ksooo commented Sep 30, 2017

jenkins errors are unrelated

@ksooo ksooo merged commit 1bca509 into xbmc:master Sep 30, 2017

1 check failed

default Sorry, building this PR failed. Please check the logs for the errors.
Details

@ksooo ksooo deleted the ksooo:pvr-database-concurrency branch Sep 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment