EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
973 stars 62 forks source link

Duplicate node / deepcopy bug in 0.13 #64

Closed mands closed 3 years ago

mands commented 3 years ago

Hi,

Hitting some issues running 0.13 - our CI tests generate the following error - which recurses into a long set of deepcopy operations, failing eventually within the pickle failure at the bottom. I haven't dug into the code but think there may perhaps be a bug in the duplicate_node that is copying additional python objects that it shouldn't be.

  File "/home/runner/work/datapane-hosted/datapane-hosted/dp-server/.venv/lib/python3.9/site-packages/django_components/component.py", line 146, in duplicate_node
    setattr(clone, nodelist_name, type(nodelist)(nodelist_contents))
AttributeError: can't set attribute

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/datapane-hosted/datapane-hosted/dp-server/.venv/lib/python3.9/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/home/runner/work/datapane-hosted/datapane-hosted/dp-server/.venv/lib/python3.9/site-packages/django/core/handlers/base.py", line 204, in _get_response
    response = response.render()
...
  File "/home/runner/work/datapane-hosted/datapane-hosted/dp-server/.venv/lib/python3.9/site-packages/django_components/component.py", line 151, in duplicate_node
    return deepcopy(source_node)
  File "/opt/hostedtoolcache/Python/3.9.5/x64/lib/python3.9/copy.py", line 172, in deepcopy
    y = _reconstruct(x, memo, *rv)
  File "/opt/hostedtoolcache/Python/3.9.5/x64/lib/python3.9/copy.py", line 270, in _reconstruct
    state = deepcopy(state, memo)
  File "/opt/hostedtoolcache/Python/3.9.5/x64/lib/python3.9/copy.py", line 146, in deepcopy
    y = copier(x, memo)
  File "/opt/hostedtoolcache/Python/3.9.5/x64/lib/python3.9/copy.py", line 230, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
...
  File "/opt/hostedtoolcache/Python/3.9.5/x64/lib/python3.9/copy.py", line 230, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
  File "/opt/hostedtoolcache/Python/3.9.5/x64/lib/python3.9/copy.py", line 161, in deepcopy
    rv = reductor(4)
TypeError: cannot pickle '_thread.lock' object
EmilStenstrom commented 3 years ago

@mands Could you help us reproduce this? Obviously we are missing a test for your use-case, but guessing what that test should be without more details is very hard :/

@Rbeard0330 You have any idea what this could be?

rbeard0330 commented 3 years ago

@mands Three questions:

It does indeed look like we're trying to deep copy something that isn't copiable (or isn't copiable between threads or processes maybe?). The code needs to make shallow copies of all of the nodes it's supposed to render, but when it can't do that (e.g., the nodelist is a synthetic property that doesn't support assignment), it falls back to a deep copy. A couple possibilities:

  1. We could have a more nuanced fallback than a deep copy. It'll be messy and introspective, but really all we need to do is copy deeply enough that we don't have any shared SlotNode instances, so maybe we can figure something out and/or bail with a clear error message if something doesn't work.
  2. We could ditch the whole copy approach and make SlotNodes smart enough to handle being rendered in multiple components. We could manage that through context, but that approach would fail if a slot is rendered without a context. There may be some way to work around that issue though... let me give it a little thought.
EmilStenstrom commented 3 years ago

@mands Please test 0.14 to see if it fixes the problem.

mands commented 3 years ago

Hi @EmilStenstrom and @rbeard0330 - sorry was afk over the weekend.

We have quite a lot of components, most are simple but some of them pass the Django/DRF Request object across as context - I think these were the ones that were failing.

Have just tried with 0.14 but we hit a similar but different issue,

  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/template/base.py", line 172, in render
    return self._render(context)
  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/test/utils.py", line 95, in instrumented_test_render
    template_rendered.send(sender=self, template=self, context=context)
  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/dispatch/dispatcher.py", line 177, in send
    return [
  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/dispatch/dispatcher.py", line 178, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/test/client.py", line 219, in store_rendered_templates
    store['context'].append(copy(context))
  File "/usr/lib64/python3.9/copy.py", line 84, in copy
    return copier(x)
  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/template/context.py", line 157, in __copy__
    duplicate = super().__copy__()
  File "/home/apoorva/Desktop/datapane/dp-server/.venv/lib/python3.9/site-packages/django/template/context.py", line 38, in __copy__
    duplicate = copy(super())
  File "/usr/lib64/python3.9/copy.py", line 72, in copy
    cls = type(x)
RecursionError: maximum recursion depth exceeded while calling a Python object

However can confirm that dropping back to 0.12 fixes things.

rbeard0330 commented 3 years ago

Thanks for the report @mands, even if it's a discouraging one? I don't see anything in the trace back that points to a line in this library... Is there more that you left off that would give us a place to start looking? If not, is it possible for you to use a debugger to figure out what the infinite recursion is doing? Many thanks!

mands commented 2 years ago

@rbeard0330 @EmilStenstrom Thanks for the reply and apologies for the delay.

I believe I have a reproducible example, the issue seems to be when a component has nested another component that has a slot with the same name.

So for instance, in our code we have a component as follows, which has an (unused in this case) slot called body.

<li {% if border_top %}class="border-t border-gray-200"{% endif %}>
  <div class="block focus:outline-none focus:bg-gray-50 transition duration-150 ease-in-out">
    <div class="py-4">
      <div class="text-lg leading-7 font-medium text-indigo-600 truncate">
        {{ header }}
      </div>
      {% slot body %}{% endslot %}
    </div>
  </div>
  {% slot extra_content %}{% endslot %}
</li>

When called from the following fragment, it includes a sub-component in the body slot

{% component_block "instruction_li" border_top=True header="header" %}
    {% slot "body" %}
        {% component "copy_field" field_selector2="line-3-field" content="content" %}
    {% endslot %}
{% endcomponent_block %}

The copy_field component eventually calls a few other components until it ends up at the following,

...
<div class="ml-3 w-0 flex-1 pt-0.5">
  <p class="text-sm leading-5 font-medium text-gray-900">
    {{ head }}
  </p>
  <p class="mt-1 text-sm leading-5 text-gray-500">
    {% slot body %}
    {% endslot %}
  </p>
</div>
...

It seems to be this nested body slot that causes the infinite recursion - renaming it to something else, like body1, fixes the issue. I've also attached the test log output - it's quite long and unsure if that helpful :)

test_log.txt

rbeard0330 commented 2 years ago

Thank you, this is extremely helpful! I'll dig into this this afternoon and see what's causing this behavior.

EmilStenstrom commented 2 years ago

I moved this new problem to a separate thread, lets continue troubleshooting there.