-
Notifications
You must be signed in to change notification settings - Fork 371
BlobWriteStream.Dispose() causes thread starvation under load #754
Comments
The proposed |
Hi @DaRosenberg, thanks for reaching out! I've logged this issue for further investigation. |
@zezha-msft I have now completed testing to confirm that this did indeed account for a significant bottleneck, and that changing it to commit asynchronously nearly doubled the scalability of our service. After that, other bottlenecks came into play that we proceeded to address. Through a combination of a few changes we made in our fork, we were able to achieve perfect scalability. I will be creating separate issues shortly for the other changes we had to make. |
Usually, the way you work around async dispose is to call FlushAsync (assuming FlushAsync also causes the commit in this case). |
@davidfowl Unfortunately |
I see CommitAsYnc is exposed directly. Is it an option to call it? Or is some other component writing that you have no control over? |
@davidfowl The class is |
Ah hah! Yes that's bad 😄 |
I think we have a solution for this. CloudBlockBlob.OpenWriteAsync() should return a CloudBlobStream, which has a public CommitAsync() method - does that not work? using (Stream stream = await blob.OpenWriteAsync()) it's entirely plausible to me that the comment would be missed. Any suggestions? |
@asorrin-msft You are right - that should work without any code change actually, because using (var stream = await blob.OpenWriteAsync())
{
// Do some writing to the stream here...
if (stream is CloudBlobStream blobStream)
await blobStream.CommitAsync();
} The issue of discoverability is something I've also been thinking about. An XML doc comment on ´OpenWriteAsync()` will not go very far I'm afraid. Might one consider taking this so far as to actually throw from |
Any plans to solve this with DisposeAsync? |
A word of caution with the using blocks around the CloudBlobStream. If there's an exception before you commit, you will end up committing partial writes or an empty stream. This would then overwrite your exist blob with either incomplete or 0 bytes of data. Ask me how I know :( |
@justinSelf I'm a bit perplexed as to how that could happen. Are you saying commits happen even before we even call |
@DaRosenberg
I'm only resurrecting this thread because of searching for this issue. |
@DaRosenberg Also, to directly answer your question, Dispose, as you know, will also call |
@justinSelf Duh, of course, stupid of me. The CommitAsync() happening as part of Dispose() was the whole reason why I opened this issue in the first place. 🤣 |
Which service(blob, file, queue, table) does this issue concern?
Blob
Which version of the SDK was used?
9.3.0
Which platform are you using? (ex: .NET Core 2.1)
.NET Core 2.1
What problem was encountered?
I've been troubleshooting a scalability issue with our ASP.NET Core Web API, which makes heavy use of blob storage for underlying storage.
We observed:
Long story short, through profiling I have finally pinpointed the issue to be here:
azure-storage-net/Lib/WindowsRuntime/Blob/BlobWriteStream.cs
Lines 200 to 220 in 38425e7
The problem is the
CommitAsync().Wait()
. Since the dispose pattern in .NET is not async, this stream implementation of course has to block on the commit operation, and the commit is necessary to ensure that the written blocks are committed to blob storage. This blocks the thread for the duration of that commit operation, which under load leads to thread starvation.Have you found a mitigation/solution?
One solution would be for our service to do
await stream.CommitAsync();
before disposing the stream, but unfortunatelyBlobWriteStream
is marked asinternal
so this type is not visible to our code. The only solution I can think of is to makeBlobWriteStream
public, allowing client code to asynchronously commit the stream before disposing it.The text was updated successfully, but these errors were encountered: