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

Correct task awaiting with ConfigureAwait(false) in Producer. #967

Merged
merged 24 commits into from
May 31, 2019

Conversation

levdimov
Copy link
Contributor

This fix resolves an issue when client code waits ProduceAsync synchronously (using .Result or .GetAwaiter().GetResult()). This fix is very useful for keeping backward compatability with legacy code which you cannot easily migrate to async. As I wrote here before 1.0.0 Producer code was returning tasks rather than awaiting them inside implementation.

I also added ConfigureAwaitChecker.Analyzer package so you will have warnings in future

@ghost
Copy link

ghost commented May 30, 2019

It looks like @levdimov hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@ghost
Copy link

ghost commented May 30, 2019

@confluentinc It looks like @levdimov just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@mhowlett mhowlett mentioned this pull request May 30, 2019
@mhowlett
Copy link
Contributor

looks good except for unnecessary white space changes (these make it difficult to track what changes happened when). also, I'm not keen on the ConfigureAwaitChecker.Analyzer dependency. I don't know why it would be a problem, but as a rule of thumb, prefer to not to have dependencies unless critical. if you make those changes, LGTM. Thanks!

@levdimov
Copy link
Contributor Author

Sorry about whitespace changes, my IDE is removing trailing whitespaces automatically and I hide all whitespace changes in Git UI :)

Also I've removed ConfigureAwaitChecker.Analyzer. Personally, I don't see any problems with private dependencies, but you are the boss :)

Thank you!

@mhowlett mhowlett changed the base branch from master to 1.0.x May 31, 2019 15:36
@mhowlett
Copy link
Contributor

thanks! I also don't see any problem, but I don't have time to give it any thought and not including it is definitely 100% safe.

@mhowlett mhowlett merged commit 431b837 into confluentinc:1.0.x May 31, 2019
@levdimov levdimov deleted the producer_configure_await branch May 31, 2019 15:44
@levdimov
Copy link
Contributor Author

@mhowlett, may I ask you when these changes are going to be released?

@mhowlett
Copy link
Contributor

I've opened #968 so we can release asap - so hopefully soon, but there is some internal debate as to how to best do rapid releases moving forward. this is a test case for that.

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

Successfully merging this pull request may close these issues.

5 participants