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

Plugins run twice in autoreload mode #2817

Closed
2 tasks done
shniubobo opened this issue Oct 30, 2020 · 3 comments · Fixed by #2818
Closed
2 tasks done

Plugins run twice in autoreload mode #2817

shniubobo opened this issue Oct 30, 2020 · 3 comments · Fixed by #2818
Labels

Comments

@shniubobo
Copy link
Contributor

  • I have read the Filing Issues and subsequent “How to Get Help” sections of the documentation.
  • I have searched the issues (including closed ones) and believe that this is not a duplicate.
  • OS version and name: Ubuntu 18.04.5 LTS (WSL)
  • Python version: 3.8.6
  • Pelican version: e4d9c41

To reproduce

Create these files:

.
├── content
│   └── 1.rst
├── pelicanconf.py
└── plugins
    └── test_plugin.py
.. content/1.rst
####
test
####

:date: 1970-01-01
:modified: 1970-01-01

TEST
# pelicanconf.py
PLUGINS = [
    'test_plugin',
]
PLUGIN_PATHS = [
    'plugins',
]
# plugins/test_plugin.py
import logging

from pelican import signals

logger = logging.getLogger(__name__)


def test_function(content):
    logger.info('test plugin loaded')
    test = content._content
    test += 'TEST'
    content._content = test
    logger.info(content._content)


def register():
    signals.content_object_init.connect(test_function)

Then run:

$ pelican -rD --logs-dedup-min-level DEBUG content
[Not showing unrelated logs]
-> test plugin loaded
-> <p>TEST</p>
  | TEST
-> test plugin loaded
-> <p>TEST</p>
  | TESTTEST

$ pelican -D --logs-dedup-min-level DEBUG content
[Not showing unrelated logs]
-> test plugin loaded
-> <p>TEST</p>
  | TEST

The plugin runs twice with -r, but only once without -r.

The casue of the issue

After a bisect, I found that commit ed1eca1 introduced this issue.

$ git checkout ed1eca16^
$ pelican -rD --logs-dedup-min-level DEBUG content
[Not showing unrelated logs]
-> test plugin loaded
-> <p>TEST</p>
  | TEST

$ git checkout ed1eca16
$ pelican -rD --logs-dedup-min-level DEBUG content
[Not showing unrelated logs]
-> test plugin loaded
-> <p>TEST</p>
  | TEST
-> test plugin loaded
-> <p>TEST</p>
  | TESTTEST

After adding this line, I found that the plugin was registered twice:

diff --git a/pelican/contents.py b/pelican/contents.py
index 594cd3b5..ff991be9 100644
--- a/pelican/contents.py
+++ b/pelican/contents.py
@@ -139,6 +139,7 @@ class Content(object):
         if 'summary' in metadata:
             self._summary = metadata['summary']

+        logger.info(str(signals.content_object_init.receivers))
         signals.content_object_init.send(self)

     def __str__(self):
$ pelican -rD --logs-dedup-min-level DEBUG content
[Not showing unrelated logs]
-> {140542259499216: <weakref at 0x7fd28a2b8b80; to 'function' at 0x7fd28b7410d0 (test_function)>, 140542259600400: <weakref at 0x7fd28b7525e0; to 'function' at 0x7fd28b759c10 (test_function)>}
-> test plugin loaded
-> <p>TEST</p>
  | TEST
-> test plugin loaded
-> <p>TEST</p>
  | TESTTEST

But why? The reason:

>>> # Implementation of ed1eca16^
>>> import sys
>>> sys.path.insert(0, 'plugins')
>>> plugin = __import__('test_plugin', globals(), locals(), str('module'))
>>> plugin_ = __import__('test_plugin', globals(), locals(), str('module'))
>>> plugin is plugin_
True
>>>
>>> # Implementation of ed1eca16
>>> import importlib
>>> spec = importlib.machinery.PathFinder.find_spec('test_plugin', ['plugins'])
>>> plugin = importlib.util.module_from_spec(spec)
>>> plugin_ = importlib.util.module_from_spec(spec)
>>> plugin is plugin_
False

How to fix

To fix it, simply avoid loading the same plugin twice:

diff --git a/pelican/plugins/_utils.py b/pelican/plugins/_utils.py
index 4e6ec3c5..699192d3 100644
--- a/pelican/plugins/_utils.py
+++ b/pelican/plugins/_utils.py
@@ -53,6 +53,11 @@ def load_legacy_plugin(plugin, plugin_paths):
     if spec is None:
         raise ImportError('Cannot import plugin `{}`'.format(plugin))
     else:
+        # Avoid loading the same plugin twice
+        try:
+            return sys.modules[spec.name]
+        except KeyError:
+            pass
         # create module object from spec
         mod = importlib.util.module_from_spec(spec)
         # place it into sys.modules cache

I will create a pull request after writing some tests for it.

@shniubobo shniubobo added the bug label Oct 30, 2020
@avaris
Copy link
Member

avaris commented Oct 30, 2020

Thanks. I would expect old module to be deleted/garbage collected since it is overridden in the sys.modules. Returning module if it is already present makes sense, but try/except is a bit unnecessary. A simple if would do:

if spec.name in sys.modules:
    return sys.modules[spec.name]

@shniubobo
Copy link
Contributor Author

Thanks. I would expect old module to be deleted/garbage collected since it is overridden in the sys.modules. Returning module if it is already present makes sense, but try/except is a bit unnecessary. A simple if would do:

if spec.name in sys.modules:
    return sys.modules[spec.name]

I used try/except to make it consistent with:

try:
# remove module from sys.modules since it can't be loaded
del sys.modules[spec.name]
except KeyError:
pass

I will use if instead in my commit.

@justinmayer
Copy link
Member

Fix for this issue is included in the just-released Pelican 4.5.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants