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

Attempt to use json.load(fd) again #624

Merged
merged 3 commits into from
Dec 9, 2019
Merged

Conversation

dagwieers
Copy link
Collaborator

@dagwieers dagwieers commented Dec 9, 2019

This PR includes:

  • Add abstraction layer for http requests
  • Add abstraction layer for decoding json
  • Use json.load(fd) again
  • Early exit in tokenresolver
  • Rename statichelper.py to utils.py
  • Selective imports
  • Small code improvements

This fixes #554
This relates to #385

@dagwieers dagwieers added bug Something isn't working enhancement New feature or request labels Dec 9, 2019
@dagwieers dagwieers added this to the v2.3.0 milestone Dec 9, 2019
@dagwieers dagwieers force-pushed the json-load branch 4 times, most recently from 48a80c7 to 0d5d0c8 Compare December 9, 2019 08:21
This PR includes:
- Use json.load(fdesc)
- Early exit in tokenresolver
@dagwieers
Copy link
Collaborator Author

I moved the non-Kodi-specific stuff (which was caching-related) to utils.py.
I would like to move the non-VRT-related stuff from statichelpers to utils.py as well.
And maybe rename statichelpers.py (with VRT-related stuff) to vrtutils.py ?

@dagwieers dagwieers force-pushed the json-load branch 4 times, most recently from eb0d48f to 3d78747 Compare December 9, 2019 12:46
@mediaminister
Copy link
Collaborator

mediaminister commented Dec 9, 2019

And maybe rename statichelpers.py (with VRT-related stuff) to vrtutils.py ?

I'm okay with rearranging functionality in different utils files.

I got the following error with 85d065a on Python 3:

2019-12-09 13:53:18.659 T:8092   ERROR: EXCEPTION Thrown (PythonToCppException) : -->Python callback/script returned the following error<--
                                             - NOTE: IGNORING THIS CAN LEAD TO MEMORY LEAKS!
                                            Error Type: <class 'NameError'>
                                            Error Contents: name 'ValueError' is not defined
                                            Traceback (most recent call last):
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\plugin.video.vrt.nu\resources\lib\addon_entry.py", line 14, in <module>
                                                run(argv)
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\plugin.video.vrt.nu\resources\lib\addon.py", line 299, in run
                                                plugin.run(argv)
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\script.module.routing\lib\routing.py", line 130, in run
                                                self._dispatch(self.path)
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\script.module.routing\lib\routing.py", line 141, in _dispatch
                                                view_func(**kwargs)
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\plugin.video.vrt.nu\resources\lib\addon.py", line 151, in programs
                                                VRTPlayer().show_episodes_menu(program=program, season=season)
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\plugin.video.vrt.nu\resources\lib\vrtplayer.py", line 244, in show_episodes_menu
                                                self._resumepoints.refresh(ttl=ttl('indirect'))
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\plugin.video.vrt.nu\resources\lib\resumepoints.py", line 37, in refresh
                                                resumepoints_json = get_cache('resume_points.json', ttl)
                                              File "C:\Users\test\AppData\Roaming\Kodi\addons\plugin.video.vrt.nu\resources\lib\utils.py", line 60, in get_cache
                                                return load(fdesc)
                                              File "C:\Program Files\Kodi\system\python\Lib\json\__init__.py", line 296, in load
                                                parse_constant=parse_constant, object_pairs_hook=object_pairs_hook, **kw)
                                              File "C:\Program Files\Kodi\system\python\Lib\json\__init__.py", line 348, in loads
                                                return _default_decoder.decode(s)
                                              File "C:\Program Files\Kodi\system\python\Lib\json\decoder.py", line 337, in decode
                                                obj, end = self.raw_decode(s, idx=_w(s, 0).end())
                                              File "C:\Program Files\Kodi\system\python\Lib\json\decoder.py", line 353, in raw_decode
                                                obj, end = self.scan_once(s, idx)
                                              File "C:\Program Files\Kodi\system\python\Lib\json\decoder.py", line 35, in __init__
                                                ValueError.__init__(self, errmsg)
                                            NameError: name 'ValueError' is not defined
                                            -->End of Python script error report<--

So, I think we need to catch JSONDecodeError instead of ValueError on Python 3.

@dagwieers
Copy link
Collaborator Author

I don't think so, ValueError should exist.
https://docs.python.org/3/library/exceptions.html#ValueError

I cannot split it up as I envisioned because of circular dependencies.
I now have statichelpers.py renamed to vrtutils.py (which is mostly ok, except for from_unicode and to_unicode)

@dagwieers
Copy link
Collaborator Author

dagwieers commented Dec 9, 2019

I don't think so, ValueError should exist.

The integration tests did not fail for this, so it ought to work. What Python version is this?

Update: The only hits I get on name 'ValueError' is not defined are from people misspelling ValueError.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Dec 9, 2019

The error does not make sense and is not something we can do anything about anyway.
JSONDecodeError is subclassed from ValueError, if ValueError would not be defined, it could not subclass from it.

I wonder if you have somewhere old .pyc files lying around that may be redefining ValueError, or something like that. If you distribute using ZIP files that can't happen, but if you copy over files, you may accidentally leave behind .pyc or .pyo or pycache files.

class JSONDecodeError(ValueError):
    """Subclass of ValueError with the following additional properties:

    msg: The unformatted error message
    doc: The JSON document being parsed
    pos: The start index of doc where parsing failed
    lineno: The line corresponding to pos
    colno: The column corresponding to pos

    """
    # Note that this exception is used from _json
    def __init__(self, msg, doc, pos):
        lineno = doc.count('\n', 0, pos) + 1
        colno = pos - doc.rfind('\n', 0, pos)
        errmsg = '%s: line %d column %d (char %d)' % (msg, lineno, colno, pos)
        ValueError.__init__(self, errmsg)
        self.msg = msg
        self.doc = doc
        self.pos = pos
        self.lineno = lineno
        self.colno = colno

    def __reduce__(self):
        return self.__class__, (self.msg, self.doc, self.pos)

@mediaminister
Copy link
Collaborator

I got this error with the built-in Python 3 in Kodi 19.0-ALPHA1 Git:20191205-b238c23acc for Windows x64

@mediaminister
Copy link
Collaborator

I got the same error again on a clean Kodi install: 19.0-ALPHA1 Git:20191209-923cfade46
Full debug log: fulldebug.txt

@dagwieers
Copy link
Collaborator Author

dagwieers commented Dec 9, 2019

What python version is this? We test almost the entire codebase (including these routines) against 3.5, 3.6, 3.7 and 3.8.

Could you copy the above definition of the JSONDecodeError class from this Python here ?
I would expect it to be identical and subclassing from ValueError.

I would say that Python is severely broken. It is contradicting itself, which is only possible if ValueError was somehow deleted during operation (del __builtins__.ValueError).

I get the same error if I do:

#!/usr/bin/python
import json
del __builtins__.ValueError
raise json.JSONDecodeError(doc='', msg='', pos=3)

results in:

[dag@moria plugin.video.vrt.nu]$ python3 test.py
Traceback (most recent call last):
  File "test.py", line 7, in <module>
    raise json.JSONDecodeError(doc='', msg='', pos=3)
  File "/usr/lib64/python3.6/json/decoder.py", line 35, in __init__
    ValueError.__init__(self, errmsg)
NameError: name 'ValueError' is not defined

This PR includes:
- Use json.load(fdesc)
- Early exit in tokenresolver
- Move caching functionality to utils.py
- Fix 2 issues
- Return default value on failure
- Fix more json.load() calls for Python 3.5
@mediaminister
Copy link
Collaborator

mediaminister commented Dec 9, 2019

What python version is this?

It should be Python 3.7 (xbmc/xbmc@61f40ea)

class JSONDecodeError(ValueError):
    """Subclass of ValueError with the following additional properties:

    msg: The unformatted error message
    doc: The JSON document being parsed
    pos: The start index of doc where parsing failed
    lineno: The line corresponding to pos
    colno: The column corresponding to pos

    """
    # Note that this exception is used from _json
    def __init__(self, msg, doc, pos):
        lineno = doc.count('\n', 0, pos) + 1
        colno = pos - doc.rfind('\n', 0, pos)
        errmsg = '%s: line %d column %d (char %d)' % (msg, lineno, colno, pos)
        ValueError.__init__(self, errmsg)
        self.msg = msg
        self.doc = doc
        self.pos = pos
        self.lineno = lineno
        self.colno = colno

    def __reduce__(self):
        return self.__class__, (self.msg, self.doc, self.pos)

@dagwieers
Copy link
Collaborator Author

dagwieers commented Dec 9, 2019

So I think this is ready to be merged.

The reported NameError seems to me something that either should happen to older VRT NU releases as well, or is related to the distribution mechanism distributing compiled objects onto a newly installed system. (BTW Even if you reinstall Kodi, the userdata directories may not be cleaned up).

@dagwieers dagwieers changed the title Attempt to use json.load(fdesc) again Attempt to use json.load(fd) again Dec 9, 2019
@mediaminister
Copy link
Collaborator

You can merge this as this is an improvement and doesn't break things.

Even if you reinstall Kodi, the userdata directories may not be cleaned up

Userdata was definitely removed, I reconfigured VRT NU add-on, activated debug logging again, etc...it really was a clean Kodi install.

I suspect the ValueError happens when xbmcvfs is doing multiple disk operations at the same time.

I can reproduce this error when starting and stopping episodes quickly after each other.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Dec 9, 2019

@mediaminister Ok, I believe you. Then the less (now most) probable cause is a bug (or at least an incompatibility) in the Python version of Kodi 19 related to reuselanguageinvoker. Either a design fault triggered by a newer Python 3, or an actual bug that can be fixed.

A minimal reproducer will probably convince developers, but if you report this I expect disbelieve and expecting a user-problem (like me :-)). It would explain why we do not see this in our integration tests.

What could also help, is recursively grepping the code for "ValueError" (and anything causing it to become undefined briefly when e.g. reloading something).

@dagwieers
Copy link
Collaborator Author

PS I doubt this is xbmcvfs related, it is more related to the inner workings of Python or how reuselanguageinvoker works.

@dagwieers dagwieers merged commit ca5c8c2 into add-ons:master Dec 9, 2019
@dagwieers
Copy link
Collaborator Author

I tried to reproduce this on a Windows 10 laptop using Kodi 19.0-ALPHA1 Git_20191018 and I couldn't reproduce the reported issue, but I managed to make Kodi crash after 7 successive playbacks of Terzake episodes 🙄

Now going to upgrade to test again.

@dagwieers
Copy link
Collaborator Author

@mediaminister I cannot reproduce this with Kodi 19.0-ALPHA1 64bit Git_20191209, can you provide more detailed instructions?

@mediaminister
Copy link
Collaborator

mediaminister commented Dec 9, 2019

Steps to reproduce:

  1. Open a menu of a program with multiple episodes.

  2. Alternately play two different episodes for the shortest time possible

    • Play episode 1 until you have video
    • Stop Player
    • Play episode 2 until you have video
    • Stop Player

Repeat the steps in 2. until you get an error.

@dagwieers
Copy link
Collaborator Author

Would probably be the easiest to disable reuselanguageinvoker and see if it can still be reproduced.

@dagwieers
Copy link
Collaborator Author

Steps to reproduce:

I tried that a lot. But ending video playback usually takes a second or more, so it's hard to actually play them with little delay. Maybe it is easier using the JSONRPC interface?

@dagwieers
Copy link
Collaborator Author

I cannot reproduce this. The fastest way to play streams is by adding a folder to the playlist (press Q on a folder, e.g. TerZake) and then you can select new video's without having to leave the listing.

@mediaminister
Copy link
Collaborator

mediaminister commented Dec 9, 2019

Okay, nevermind, I just tested this in Virtualbox, I'll open a new issue if I can reliably reproduce this on another (Windows) system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No json object could be decoded
2 participants