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

[BUG]: Risk for deadlock when ParquetWriter is disposed? #479

Closed
andagr opened this issue Feb 23, 2024 · 4 comments
Closed

[BUG]: Risk for deadlock when ParquetWriter is disposed? #479

andagr opened this issue Feb 23, 2024 · 4 comments
Assignees
Milestone

Comments

@andagr
Copy link

andagr commented Feb 23, 2024

Library Version

4.23.4

OS

Windows

OS Architecture

64 bit

How to reproduce?

This is not easy to reproduce because it depends on where the library is used, behaviour is different depending on if it's running e.g in a GUI app or ASP.NET. In my case it is used in an Azure Function app, and out of several thousand serializations per day the process hangs a few times.

ParquetWriter.Dispose() contains a synchronous wait on a task without .ConfigureAwait(false). This is known to cause deadlocks and I suspect this is causing our function app to hang. More info on ConfigureAwait:
https://devblogs.microsoft.com/dotnet/configureawait-faq/#why-would-i-want-to-use-configureawaitfalse
On the other hand it seems Task.Run() is also possible to use for avoiding deadlocks since it puts the work in a background thread:
https://devblogs.microsoft.com/dotnet/configureawait-faq/#can-i-use-task-run-to-avoid-using-configureawaitfalse
https://stackoverflow.com/a/28307965/114174

Here is when the synchronous wait was added:
1d1ced2#diff-67e525645019c5bdbfa30dd0862bf24bccea0d8d5bd4e95a37c06975d1d41e5eR145

Either way I'm not sure about this, I will change this to IAsyncDisposable in a local build instead and see if it resolves the hanging process on our end.

Failing test

No response

@andagr andagr changed the title [BUG]: Risk for deadlock when ParquetWriter is disposed [BUG]: Risk for deadlock when ParquetWriter is disposed? Feb 23, 2024
@andagr
Copy link
Author

andagr commented Feb 23, 2024

Here are the changes I've done and will try out:
master...andagr:parquet-dotnet:master

@aloneguid
Copy link
Owner

Good point. It was created when IAsyncDisposable did not exist. If I'm not wrong await using is suppoted by most C# version, so this can be added, unfortunately as a breaking change.

@andagr
Copy link
Author

andagr commented Mar 19, 2024

@aloneguid Unfortunately this hasn't resolved our function timeouts completely, so at the very least this is not the only reason for them.

Still, it would be nice to support IAsyncDisposable now that it does exist, would it be ok if I add a PR for this? If yes, how would you prefer this? Some alternatives:

  1. Replace usage of IDisposable with IAsyncDisposable and accept a breaking change/new major version
  2. Keep IDisposable and add IAsyncDisposable support to ParquetWriter, avoid breaking change by:
    a. Add a new argument to ParquetSerializer.SerializeAsync to be able to choose between IDisposable or IAsyncDisposable, default to IDisposable to keep backwards compatibility
    b. Add a new method similar to ParquetSerializer.SerializeAsync, but which uses IAsyncDisposable (what should it be called?)

@aloneguid aloneguid self-assigned this Jun 6, 2024
@aloneguid aloneguid added this to the 5.0.0 milestone Oct 2, 2024
aloneguid added a commit that referenced this issue Oct 2, 2024
- Refactored ParquetWriter to implement IAsyncDisposable, added DisposeAsync and WriteMagicAsync methods, and refactored Dispose method.
- Updated ParquetWriterTest.cs and ParquetSerializer.cs to use await using for async disposal.
- Added WriteInt32Async method to StreamExtensions.cs. Added synchronous Write method to ThriftFooter.cs. Removed BinaryWriter field and property from ParquetActor.cs.
@aloneguid
Copy link
Owner

Released.

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

No branches or pull requests

2 participants