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

Picframe stalls with error #153

Closed
junglebells opened this issue Jul 28, 2021 · 21 comments · Fixed by #155
Closed

Picframe stalls with error #153

junglebells opened this issue Jul 28, 2021 · 21 comments · Fixed by #155
Assignees
Labels
bug Something isn't working

Comments

@junglebells
Copy link

This has now happened a couple of times. The picframe would error out with the following message. The debug file is on "Warning" but did not have anything in it. Any thoughts?

Traceback (most recent call last): File "/home/pi/.local/bin/picframe", line 10, in <module> sys.exit(main()) File "/home/pi/.local/lib/python3.7/site-packages/picframe/start.py", line 157, in main c.loop() File "/home/pi/.local/lib/python3.7/site-packages/picframe/controller.py", line 284, in loop pics = self.__model.get_next_file() File "/home/pi/.local/lib/python3.7/site-packages/picframe/model.py", line 297, in get_next_file pic_row = self.__image_cache.get_file_info(file_ids[0]) File "/home/pi/.local/lib/python3.7/site-packages/picframe/image_cache.py", line 147, in get_file_info self.__update_file_stats(file_id) File "/home/pi/.local/lib/python3.7/site-packages/picframe/image_cache.py", line 159, in __update_file_stats self.__db.execute(sql, (file_id,)) sqlite3.OperationalError: cannot start a transaction within a transaction

@jgodfrey
Copy link
Collaborator

Thanks for the report.

Off the top of my head...

The __update_file_stats call updates some image stats in the database for the current image - while the slideshow is running. I'd guess that update must be colliding with the image caching process - which also updates the db (though, I'm not sure that explains the transaction within a transaction message).

If that's the cause, we might need to simply store the image stat updates in memory and wait to push them into the db during the existing cache update process.

It'll take some more digging to be sure of the cause.

@jgodfrey
Copy link
Collaborator

Yeah, I do think what I described above is likely the issue, although I'm surprised it hasn't been seen or reported before. I think we're (unintentionally) writing to the db using two separate threads. Apparently, we get away with it most of the time. I think my suggestion of simply storing the stat updates in memory and letting the existing db write thread do the actual db updates will fix the issue.

If anyone has other suggestions, I'm listening. Otherwise, I'll try to look into a fix soon.

@jgodfrey
Copy link
Collaborator

@junglebells - Are you running picframe from the normal, released package or from source code? And, if from source, which branch are you using?

@junglebells
Copy link
Author

junglebells commented Jul 28, 2021 via email

@junglebells
Copy link
Author

Here is the version: pi3d-2.46 picframe-2021.7.5

Hi @jgodfrey - while we are looking at this, I also wanted to report that it takes a long time (several hours to days) for location data (reverse geo) to get populated in the db. I had about 800 pictures with about 100 of them with gps info. It would be nice to have this speed up, to use a location based filter used properly. thanks!

@jgodfrey
Copy link
Collaborator

I also wanted to report that it takes a long time (several hours to days) for location data (reverse geo) to get populated in the db. I had about 800 pictures with about 100 of them with gps info.

Yeah, I'd guess this is because the GPS info is only resolved (for the first time) when the image is first displayed. So, for the entire playlist to have complete GPS data, it has to be shown in its entirely once (ignoring any potential sharing of location data between multiple images from the same location).

There's some additional discussion here.

There's definitely room for improvement in this area...

@junglebells
Copy link
Author

Yeah, I'd guess this is because the GPS info is only resolved (for the first time) when the image is first displayed. So, for the entire playlist to have complete GPS data, it has to be shown in its entirely once (ignoring any potential sharing of location data between multiple images from the same location).
There's some additional discussion here.
There's definitely room for improvement in this area...

Thank you @jgodfrey. I like the potential solution mentioned here: #82 , hope it can also be prioritized :)

@jgodfrey jgodfrey added the bug Something isn't working label Jul 29, 2021
@jgodfrey jgodfrey self-assigned this Jul 29, 2021
@jgodfrey
Copy link
Collaborator

jgodfrey commented Jul 29, 2021

FYI - I think I have this fixed locally, though since I've never actually seen the problem, I guess I can't be sure. However, I've moved the db update of the image stats into the caching thread, which really is the only place we should be doing db updates in the first place. So, I'm fairly confident... :)

Hmmm... Looking closer...

@paddywwoof - If this issue really is caused by writing to the db from 2 separate threads (which is my guess), it looks like we could have the same possibility when adding location data (image_cache.__get_geo_location()). That's called from __get_file_info(), which is also called on the main thread.

Any thoughts?

@paddywwoof
Copy link
Collaborator

Hi, I'm away at the moment but will have a look at this when I get back. I vaguely thought we had convinced ourselves that it would be OK to open the db in multithread mode but can't remember why. Maybe originally there was only one place where writes happened. The actual cause of the error might be tied up with something causing the update to happen very slowly on @junglebells system, no idea why though, I've not seen any similar errors.

There's a kind of semaphore system to stop the file updating in image_cache while the slides are transitioning, maybe that could be used to get geo location and avoid clashes (without actually looking at the code!)

Paddy

@jgodfrey
Copy link
Collaborator

Thanks for the input @paddywwoof. I'll commit a separate branch soon with my initial attempt at fixing the reported issue. It's really fairly simple and something similar could probably be done with the location data if it seems reasonable - though in that case, we might want to make some of my changes a bit more general to handle both scenarios.

Basically, instead of writing the file stat updates directly to the db (from the main thread), I'm adding them to a collection that's shared between the two threads (with appropriate locking I hope). So, the main thread adds new file stat info to the collection and the cache thread periodically inserts those items into the db and drains the collection.

Anyway, I'll keep an eye out for more input from you when your around.

jgodfrey added a commit that referenced this issue Jul 30, 2021
image_cache.py
- Move database updates for image stats from the main thread into the
  existing image caching thread. The main thread shouldn't update the db.
- Added some shared collection locking for safe thread access.

controller.py
- Also, fix an issue where the model.pause_looping flag was being set
  opposite of what was intended. This had the effect of running some
  database updates while the image was in transition instead of when
  it was idle.
@jgodfrey
Copy link
Collaborator

I just checked in a test branch with my (supposed) fix for this issue. That's here: https://github.com/helgeerbe/picframe/tree/Issue-153%2C-DB-Error.

While working on this, I found a place where I think we set the loop pausing mechanism (to allow db updates when the compute load is light) to the opposite of what was intended. This resulted in allowing db cache updates to occur during the image transition stage instead of the idle stage (doh!).

@paddywwoof - check out the change here and let me know if you agree with it. It looks like a big, glaring oversite to me though...

Once we get some consensus here, I can merge this into dev.

@jgodfrey
Copy link
Collaborator

jgodfrey commented Aug 6, 2021

I'll likely merge the changes from the above-mentioned branch into dev soon unless anyone has a reason not to.

@junglebells
Copy link
Author

@jgodfrey I'd be happy to test in dev if possible, when code is merged

jgodfrey added a commit that referenced this issue Aug 8, 2021
@jgodfrey
Copy link
Collaborator

jgodfrey commented Aug 8, 2021

Seeing no other input, I just merged these changes with dev (#155).

@junglebells - if you're able to try it out, that'd be great.

@junglebells
Copy link
Author

@junglebells - if you're able to try it out, that'd be great.

@jgodfrey - installed the dev branch and started picframe, will let you know how it goes. Thanks so much for your help!

@junglebells
Copy link
Author

@jgodfrey - just wanted to let you know that the picframe has been running successfully for last few days without any errors or crashes. Looks like your magic fix is working :)

@jgodfrey
Copy link
Collaborator

@junglebells - Nice! Thanks for the update. Are you relatively confident that you would have seen the problem by now without the fix? Once you're comfortable that things are working correctly, we can probably close this issue. Thanks again.

@junglebells
Copy link
Author

I am fairly confident that we would have seen the problem by now, but I am going to leave the test running through the weekend. How about we close the issue then? Thanks again @jgodfrey

@jgodfrey
Copy link
Collaborator

@junglebells - That works for me. Keep us posted. Thanks!

@junglebells
Copy link
Author

@jgodfrey - it's been over a week and the system has been running great!

It's okay to close the issue. Thanks again for your help.

@jgodfrey
Copy link
Collaborator

Thanks @junglebells. Done.

@jgodfrey jgodfrey linked a pull request Aug 16, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants