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

Do not allow sending events responses while handling RPC messages #4617

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Sep 9, 2024

Fixes an issue while running viewing a small polars data frame. In this case, the background tasks that return the null cournts would be processed faster than the actual RPC message, leading to an issue where the send_event is sent before send_result, but with the message id from send_result, which cause positron to interpret the event and the return value of the RPC message.

This PR makes sure send_event waits for for the RPC message to be handled before sending the event results.

import polars as pl

from datetime import date
df = pl.DataFrame(
    {
        "foo": [1, 2, 3],
        "bar": [6.0, 7.0, 8.0],
        "ham": [date(2020, 1, 2), date(2021, 3, 4), date(2022, 5, 6)],
        "a": [None, 2, 3],
        "b": [0.5, None, 2.5],
        "c": [True, None, False],
    }
)


%view df

@dfalbel dfalbel requested a review from wesm September 9, 2024 19:00
Copy link
Contributor

@wesm wesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems correct to me — the behavior can only be triggered when events may be fired from a background thread (i.e. with #4326) while a request from the main request handling loop is in progress

@dfalbel dfalbel merged commit 545bfd6 into feature/de-async-column-profiles Sep 9, 2024
1 check passed
@dfalbel dfalbel deleted the bugfix/lock-sending-while-handling-rpc branch September 9, 2024 19:11
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants