Skip to content

plugin: remove Plugin.bind(), change Plugin.__init__() and Session.resolve_url() #4768

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

Merged
merged 4 commits into from
Aug 28, 2022

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Aug 25, 2022

Resolves #4744
Fixes #4358

This introduces breaking changes to the Plugin.__init__() method, as well as Session.resolve_url() and Session.resolve_url_no_redirect().

See the commit messages of each individual commit for proper descriptions and explanations.

Current master:

>>> import streamlink
>>> import gc
>>> streamlink.__version__
'4.3.0+18.g60e87243'
>>> s = streamlink.Streamlink()
>>> del s
>>> gc.collect()
0
>>> [obj for obj in gc.get_objects() if isinstance(obj, streamlink.Streamlink)]
[<streamlink.session.Streamlink object at 0x7f59380a36d0>]

Removal of Plugin.bind():

>>> import streamlink
>>> import gc
>>> streamlink.__version__
'4.3.0+21.gf34a33cc'
>>> s = streamlink.Streamlink()
>>> del s
>>> gc.collect()
3835
>>> [obj for obj in gc.get_objects() if isinstance(obj, streamlink.Streamlink)]
[]

I've decided to add compatibility wrappers for old-style Plugin constructors, so that breaking changes for custom plugin implementations can be avoided for a short while and deprecation messages be shown.

I still need to update the docs (which I just remembered while writing this). But even then, I want to keep this PR open for a bit, so I can think about all this and check if something's wrong or missing.

@bastimeyer
Copy link
Member Author

Btw, as a next step (ignore this for now in this PR though), since the session is not a class attribute on the Plugin class anymore and is thus not defined as None initially, typing information can finally be added, so that calls to self.session.http.get() and the likes can be resolved by the IDE/editor and static code analyzers like mypy.

I have stashed some local changes, but these changes also required adding wrappers for direct HTTP-verb-methods of Streamlink's HttpSession subclass, like get, post, head, patch, put, options and delete, as the schema keyword and others are not recognized due to the typing stubs of requests. Otherwise, it shows typing warnings when calling self.session.http.get(url, schema=...).

@bastimeyer bastimeyer force-pushed the plugin/remove-bind branch 3 times, most recently from be20ca2 to e2e1eb8 Compare August 27, 2022 09:41
@bastimeyer
Copy link
Member Author

Rebased to master and fixed merge conflicts introduced by #4771

@bastimeyer bastimeyer force-pushed the plugin/remove-bind branch 2 times, most recently from a73cc22 to e2e1eb8 Compare August 28, 2022 09:15
@bastimeyer

This comment was marked as outdated.

and add `Plugin.__init__` signature test for built-in plugins
This changes the way how the Streamlink session and other objects like
the plugin cache and logger are stored on each plugin.

Previously, those objects were set as class attributes on every `Plugin`
class via `Plugin.bind()` when loading plugins via the session's
`load_plugins()` method that gets called on initialization.

This meant that whenever a new Streamlink session was initialized,
references to it (including a dict of every loaded plugin) were set
on each `Plugin` class as a class attribute, and Python's garbage
collector could not get rid of this memory when deleting the session
instance that was created last.

Removing `Plugin.bind()`, passing the session via the `Plugin.__init__`
constructor, and setting the cache, logger, etc. on `Plugin` instances
instead (only one gets initialized by `streamlink_cli`), removes those
static references that prevent the garbage collector to work.

Since the plugin "module" name now doesn't get set via `Plugin.bind()`
anymore, it derives its name via `self.__class__.__module__` on its own,
which means a change of the return type of `Streamlink.resolve_url()`
is necessary in order to pass the plugin name to `streamlink_cli`,
so that it can load config files and initialize plugin arguments, etc.

Breaking changes:

- Remove `Plugin.bind()`
- Pass the `session` instance via the Plugin constructor and set the
  `module`, `cache` and `logger` on the plugin instance instead.
  Derive `module` from the actual module name.
- Change the return type of `Session.resolve_url()` and include the
  resolved plugin name in the returned tuple

Other changes:

- Remove `pluginclass.bind()` call from `Session.load_plugins()` and
  use the loader's module name directly on the `Session.plugins` dict
- Remove initialization check from `Plugin` cookie methods
- Update streamlink_cli.main module according to breaking changes
- Update tests respectively
  - Add explicit plugin initialization test
  - Update tests with plugin constructors and custom plugin names
  - Move testplugin override module, so that it shares the same module
    name as the main testplugin module. Rel `Session.load_plugins()`
  - Refactor most session tests and replace unneeded `resolve_url()`
    wrappers in favor of calling `session.streams()`
This restores support for custom plugins with constructors which only
take the formal `url` argument instead of variable arguments via
`*args, **kwargs` that get then passed to the super-class's constructor,
so a breaking API change can temporarily be avoided.

This is done by wrapping the custom plugin class's MRO with two
compatibility classes via `__new__()`.

Also add a deprecation message (info log channel) and add tests.
@bastimeyer
Copy link
Member Author

Rebased to master now after #4780 was merged. No need to unnecessarily wait for this.

This is ready for review now.

@bastimeyer bastimeyer marked this pull request as ready for review August 28, 2022 09:33
@gravyboat
Copy link
Member

Looks good, thanks for the thorough commit message on e88750, made it a lot more clear.

@gravyboat gravyboat merged commit 20ec908 into streamlink:master Aug 28, 2022
@bastimeyer bastimeyer deleted the plugin/remove-bind branch August 28, 2022 19:48
Billy2011 added a commit to Billy2011/streamlink-27 that referenced this pull request Sep 3, 2022
and add `Plugin.__init__` signature test for built-in plugins
Billy2011 added a commit to Billy2011/streamlink-27 that referenced this pull request Sep 14, 2022
and add `Plugin.__init__` signature test for built-in plugins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin: remove Plugin.bind classmethod plugin: session memory leak
2 participants