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 deadlock when erroring writes are slow #16937

Merged

Conversation

MasslessParticle
Copy link
Contributor

c.getErr can return before slow writes complete. If there is more than one write and those writes result in errors, the second write blocks on a full errCh. The result is a deadlocked copyFromReader call because the wait groups never clear.

The test included tests demonstrates the deadlock.

This PR causes close to simultaneously drain the errCh while it waits for writes to complete. It combines any errors that are returned.

I looked at the CHANGELOG, but not sure this change fits. Let me know if I need to do something else.

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
    • These files are managed by the codegen framework at [Azure/autorest.go][].
  • Tests are included and/or updated for code changes.
  • Updates to [CHANGELOG.md][] are included.
  • MIT license headers are included in each file.

@ghost ghost added Storage Storage Service (Queues, Blobs, Files) customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jan 28, 2022
@ghost
Copy link

ghost commented Jan 28, 2022

Thank you for your contribution MasslessParticle! We will review the pull request and get back to you soon.

@ghost
Copy link

ghost commented Jan 28, 2022

CLA assistant check
All CLA requirements met.

@MasslessParticle
Copy link
Contributor Author

@mohsha-msft @seankane-msft Hello all! Is there any word on this PR?

This fixes a goroutine leak that brings down our containers so we're excited to merge a fix and be off a fork. Let me know if I can help!

@mohsha-msft
Copy link
Contributor

mohsha-msft commented Feb 7, 2022

Hey @MasslessParticle ,

Thanks a lot for your contribution to our repo.
Apologies for I couldn't include your changes in next release 0.3.0. I'll review this PR and include this in release 0.4.0 in March. But in the meantime, I'll merge the changes in an internal azblob dev branch so if you need it, you can pull in that specific branch to your code. Does that sound good to you?

@MasslessParticle
Copy link
Contributor Author

Thanks for your reply!

No need to merge to a dev branch. I'm excited for the 0.4.0 release

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Apr 15, 2022
@ghost
Copy link

ghost commented Apr 15, 2022

Hi @MasslessParticle. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@MasslessParticle
Copy link
Contributor Author

@mohsha-msft is there still interest in accepting this PR?

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Apr 15, 2022
@azure-sdk azure-sdk deleted the branch Azure:main May 18, 2022 16:08
@azure-sdk azure-sdk closed this May 18, 2022
@jhendrixMSFT jhendrixMSFT reopened this May 18, 2022
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Jul 22, 2022
@ghost
Copy link

ghost commented Jul 22, 2022

Hi @MasslessParticle. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@MasslessParticle
Copy link
Contributor Author

@jhendrixMSFT @mohsha-msft Is there still interest in this PR?

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Jul 22, 2022
@jhendrixMSFT jhendrixMSFT removed the request for review from seankane-msft August 15, 2022 17:27
@jhendrixMSFT jhendrixMSFT force-pushed the tpatterson/fix-write-deadlock branch from a204a13 to 2a3335d Compare September 20, 2022 21:50
@jhendrixMSFT jhendrixMSFT force-pushed the tpatterson/fix-write-deadlock branch from 2a3335d to 26961ea Compare September 20, 2022 21:53
@jhendrixMSFT jhendrixMSFT removed the request for review from mohsha-msft September 21, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants