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

fix(rust): Fix sink_ipc_cloud panicking with a Tokio runtime error #14262

Closed
wants to merge 4 commits into from

Conversation

Qqwy
Copy link
Contributor

@Qqwy Qqwy commented Feb 4, 2024

Fixes #13614

When writing ot ObjectStore-compatible storage in the IPC format, it seems like the .block_on calls inside the constructed CloudWriter might sometimes get called inside another block_on call. Tokio does not like this, resulting in a panic.

This PR resolves this issue by using block_on_potential_spawn in the necessary places instead.

make CloudWriter call `get_runtime().block_on` from a newly spawned thread.
@github-actions github-actions bot added the fix Bug fix label Feb 4, 2024
@Qqwy Qqwy force-pushed the 13614-ipc_cloud_tokio_error2 branch from ef4ae59 to 87af454 Compare February 4, 2024 18:47
@Qqwy Qqwy force-pushed the 13614-ipc_cloud_tokio_error2 branch from e8f3e67 to eca2306 Compare February 4, 2024 19:36
@Qqwy Qqwy marked this pull request as ready for review February 4, 2024 19:53
@stinodego stinodego changed the title fix(rust) sink_ipc_cloud panicing with a Tokio runtime error (#13614) fix(rust): Fix sink_ipc_cloud panicking with a Tokio runtime error Feb 18, 2024
@github-actions github-actions bot added the rust Related to Rust Polars label Feb 18, 2024
@josevalim
Copy link

Hi @stinodego! I am sorry for the ping, but is there something blocking this PR or something we could do to get it merged? Thank you! ❤️

@Qqwy
Copy link
Contributor Author

Qqwy commented Jun 21, 2024

I'm still available to (help) fix(ing) the merge conflicts once more if needed, but it would be nice if it can be merged soon afterwards before new conflicts would arise 😅

@philss
Copy link
Contributor

philss commented Aug 6, 2024

I think this is now conflicting with my fix from #18027
I'm also available to help if needed :)

It seems that this fix would also fix the issue #13163

@ritchie46
Copy link
Member

I think this one can be closed as there has changed a lot since then.

@ritchie46 ritchie46 closed this Aug 7, 2024
@josevalim
Copy link

Is a fix still accepted? The functionality is currently broken and we would be happy users of it. Thank you :)

@ritchie46
Copy link
Member

Yes, it is accepted. But I believe it was fixed by #18027

@philss
Copy link
Contributor

philss commented Aug 7, 2024

I'm afraid this was not fixed yet. I could reproduce the error with the latest version from the main branch. Here is the backtrace: https://gist.github.com/philss/a81fab6f9cf85d23a0f1c364633e992b

philss added a commit to philss/polars that referenced this pull request Aug 8, 2024
Fixes pola-rs#13614

When writing to ObjectStore-compatible storage using the IPC format,
it seems like the `block_on` calls inside the constructed `CloudWriter`
might sometimes get called inside another `block_on` call.
Tokio does not like this, resulting in a panic.

This PR resolves this issue by using `block_on_potential_spawn`
in the necessary places instead.

This is a fix that was originally written by @Qqwy in another PR:
pola-rs#14262

Co-Authored-By: Qqwy / Marten <[email protected]>
philss added a commit to philss/polars that referenced this pull request Aug 8, 2024
Fixes pola-rs#13614

When writing to ObjectStore-compatible storage using the IPC format,
it seems like the `block_on` calls inside the constructed `CloudWriter`
might sometimes get called inside another `block_on` call.
Tokio does not like this, resulting in a panic.

This PR resolves this issue by using `block_on_potential_spawn`
in the necessary places instead.

This is a fix that was originally written by @Qqwy in another PR:
pola-rs#14262

Co-Authored-By: Qqwy / Marten <[email protected]>
philss added a commit to philss/polars that referenced this pull request Aug 8, 2024
Fixes pola-rs#13614

When writing to ObjectStore-compatible storage using the IPC format,
it seems like the `block_on` calls inside the constructed `CloudWriter`
might sometimes get called inside another `block_on` call.
Tokio does not like this, resulting in a panic.

This PR resolves this issue by using `block_on_potential_spawn`
in the necessary places instead.

This is a fix that was originally written by @Qqwy in another PR:
pola-rs#14262

Co-Authored-By: Qqwy / Marten <[email protected]>
philss added a commit to philss/polars that referenced this pull request Aug 8, 2024
Fixes pola-rs#13614

When writing to ObjectStore-compatible storage using the IPC format,
it seems like the `block_on` calls inside the constructed `CloudWriter`
might sometimes get called inside another `block_on` call.
Tokio does not like this, resulting in a panic.

This PR resolves this issue by using `block_on_potential_spawn`
in the necessary places instead.

This is a fix that was originally written by @Qqwy in another PR:
pola-rs#14262

Co-Authored-By: Qqwy / Marten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sink IPC cloud panics with "Tokio runtime error"
4 participants