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

Crash loop fix and other changes #182

Merged
merged 13 commits into from Feb 28, 2019

Conversation

@phunkyfish
Copy link
Member

commented Feb 7, 2019

v3.17.1

  • Fixed: Ensure only one call to GetEPGForChannel happens at one time, fixes #181
  • Added: Update/fix server version for pvr addon
  • Added: Option to skip initial EPG Load
  • Fixed: Fix for zap on channel change for dreamboxes
  • Added: Support for padding in certain timer types
  • Fixed: Change the call web/tunersignal to web/signal so it also works on DreamOS

Crashlog shows 2 calls on two different thread to GetEPGForChannel on LibreElec, reproduced on both x86 and Rpi.

@phunkyfish phunkyfish requested a review from ksooo Feb 7, 2019

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@ksooo a nice small PR. Not sure why this only happens on LibreElec. I think it's been around a while just happens occasionally.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

So the failed fix brings us from crash loop to a deadlock and only on LibreElec (before and after the fix). I'll need to play this one a bit to figure it out.

@MilhouseVH

This comment has been minimized.

Copy link

commented Feb 8, 2019

I can't think of any specific differences in the threading model used by LibreELEC that would cause this issue. As is often the case with these things it may just be down to timing, or other startup add-on activity in LibreELEC means it is more likely to trigger. I'd put money on the same issue being present in (for instance) Ubuntu, but it may simply be harder to reproduce.

src/Enigma2.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.h Outdated Show resolved Hide resolved
@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

I can't think of any specific differences in the threading model used by LibreELEC that would cause this issue.

Agreed. The problem is platform agnostic but for some reason LE seems to trigger this more often than other platforms.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

The way the addon currently works it first loads now and next EPG data followed by full EPG all based off the GetEPGForChannel call. I could try to load the now and next beforehand so it’s simply a read when GetEPGForChannel is called.

Maybe as a first step I’ll disable the now and next and see if that makes a difference. I can repro it consistently now I just need to use the testbuilds as I can’t build libreElec myself.

@ksooo the PR currently is a hack so I can try to find a fix it’s by no means finished and will have several interations while I figure it out. Will make sure it’s all cleaned up once I get it sorted.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

@ksooo I figure the latest will fix the issue. I moved the initial EPG load to before (also much quicker now), so it can be a simple read if called. So no contention across groups which caused the previous problem.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

For second last commit I missed the call to GetServerVersion. Will push the change shortly. Will post back once ready

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

Ok, good to go

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Fix confirmed in testbuilds, so we should be good. Thanks again for including @MilhouseVH

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

There's a bug in the update server version commit. Will update it shortly.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@ksooo this is finally ready to go! A not so small PR after all ;)

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Another crash loop unfortunately, will try skipping initial EPG load.

@phunkyfish phunkyfish changed the title Crash loop fix Crash loop fix and other changes Feb 18, 2019

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

So I added a default option to skip the initial EPG load (now and next) which successfully avoids the Crash Loop issue. Will revisit and look for cleaner solution when I work on the Async Connection feature.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

So @ksooo this is ready to go when you are ;)

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

There is a bug in the padding commit. Will post back once fixed.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

@ksooo, all fixes tested by users in testbuilds. Ready for your review when you have the time.

@ksooo
Copy link
Member

left a comment

Wow, now my eyes hurt.

src/Enigma2.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.cpp Show resolved Hide resolved
src/enigma2/Epg.cpp Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
pvr.vuplus/changelog.txt Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
pvr.vuplus/addon.xml.in Show resolved Hide resolved
src/enigma2/data/Timer.cpp Outdated Show resolved Hide resolved
@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

@ksooo I couldn't cleanly fixup the EPG changes so added a cleanup commit. Other than that we should be good.

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Reminder ping!

@ksooo
Copy link
Member

left a comment

nitpicking

src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
src/enigma2/Epg.cpp Outdated Show resolved Hide resolved
@ksooo
ksooo approved these changes Feb 28, 2019
Copy link
Member

left a comment

Good job (like always). 👍

@phunkyfish phunkyfish merged commit a445bf3 into kodi-pvr:master Feb 28, 2019

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@phunkyfish phunkyfish deleted the phunkyfish:crash-loop-fix branch Feb 28, 2019

@phunkyfish

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Ty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.