Skip to content

display() sends the output to the wrong tag #878

@antocuni

Description

@antocuni

<rant>
During the infinite discussion in #769 we decided to use a "dynamic" implicit target for display(), which as far as I know can be only implemented with a global variable.

I am still convinced that this approach is very fragile and it will create tons of problems and nasty and subtle bugs.
I predict this is the first of many 🔮

We are still in time to change our minds.
</rant>

I found this problem during an unrelated refactoring. I think I know what happens but I don't want to solve it now, so I'm writing an issue to make sure we don't forget.
Consider this test:

        self.pyscript_run(
            """
            <py-script>
                display('hello 1')
            </py-script>
            <p>hello 2</p>
            <py-script>
                display('hello 3')
            </py-script>
        """
        )

It displays hello 2, hello 1, hello 3:
image

I think the bug was introduce by PR #749 but the roots of the bug are much deeper than this and shows some criticality on our code base.

To start with, the current test that we have is suboptimal:

def test_consecutive_display(self):
self.pyscript_run(
"""
<py-script>
display('hello 1')
</py-script>
<py-script>
display('hello 2')
</py-script>
"""
)
# need to improve this to get the first/second input
# instead of just searching for it in the page
inner_html = self.page.content()
first_pattern = r'<div id="py-.*?-2">hello 1</div>'
assert re.search(first_pattern, inner_html)
second_pattern = r'<div id="py-.*?-3">hello 2</div>'
assert re.search(second_pattern, inner_html)
assert first_pattern is not second_pattern

it seems to test for the right thing, but because of the mess that we do when generating outputs, the test passes "by chance". The problem is that to display outputs we create <div>s inside <div>s inside <div>s, each with a random ID, which makes everything very hard to test. We should improve testability of such a simple thing. This will hopefully be solved as part of the PR I'm working on.

The other problem is much deeper. I think that the root cause is here:

try {
<string>await runtime.run(`set_current_display_target(target_id="${this.id}")`);
<string>await runtime.run(source);
} finally {
<string>await runtime.run(`set_current_display_target(target_id=None)`);
}

The thing to note is that Runtime.run is async, so the three calls are not executed in strict order. I think that what happens is roughly the following:

  1. main.ts starts to execute the first py-script. We run set_current_display_target(FIRST_TAG), but since it's async we immediately interrupt the execution
  2. main.ts starts to execute the second py-scritp. We run set_current_display_target(SECOND_TAG).
  3. at some point in the future we execute the rest of the run(), but now the "current display target" is always SECOND_TAG.

The proper fix is to ensure that the three calls are done atomically, but it's very hard to to it reliably.
A quick&dirty fix is probably to concatenate the three strings into one big string, but it's a very slipper slope. It might work right now because we execute everything in the global namespace (and we shouldn't) and it will break as soon as we stop doing it (e.g. to implement custom namespaces).

Another "maybe fix" is to use the famous ContextVar, but AFAIK it's a python-only thing. I don't really know how it interacts with the JS event loop and I think we should be very cautious before using it without fully understanding how it works in pyodide.

Another possible fix is to use synchronous calls to run: this probably solves the problem, but it breaks all py-script tags which uses await at top level. FWIW, this is one more data point which indicates that maybe they should not be allowed.
Note that currently we don't have any way to run pyodide code synchronously, which should probably kill Runtime.run and introduce Runtime.runSync and Runtime.runAsync.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    Status
    Closed

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions