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

Missing ConfigureAwait(false) on async calls #198

Closed
roryprimrose opened this issue Feb 25, 2016 · 9 comments
Closed

Missing ConfigureAwait(false) on async calls #198

roryprimrose opened this issue Feb 25, 2016 · 9 comments
Labels
status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap

Comments

@roryprimrose
Copy link

I there, I'm looking at migrating to SendGrid and was browsing through the C# support. I noticed you are using async/await (great!) but that the calls did not use .ConfigureAwait(false). This is going to cause unnecessary thread context synchronization. Can you please add ConfigureAwait calls into all the await executions.

@samrueby
Copy link

I agree. Just caught this because it resulted in deadlock in my application.

@Jericho
Copy link

Jericho commented Mar 22, 2016

I have forked SendGrid's repo and added several improvements such as ability to inject httpclient for unit testing, meaningful method names (as opposed to simply 'get', 'post', 'patch', etc.), strongly typed return values from methods, etc. Also, all methods are awaitable and ConfigAwait(false) is specified on every async call.

You can find my fork here: https://github.com/Jericho/sendgrid-csharp

@thinkingserious
Copy link
Contributor

Hello @roryprimrose, @samrueby and @Jericho,

We are in the process of releasing a new version of this library that supports all of our Web API v3 endpoints. It will be a breaking change and we are hopeful the release will happen within a few months.

The general idea is to provide a solid core that has as few third party dependencies as possible. Using this core, we will build helper functions and workflows to complete specific tasks on top of the core.

To start, we have ripped out the client into it's own repo (the new library will only have this repo and maybe our smtpapi client as dependencies), any feedback you may have is appreciated: https://github.com/sendgrid/csharp-http-client

For reference, the new library will be similar to https://github.com/sendgrid/sendgrid-python; however, there are few key things still missing in that library:

  • updated sample/example data based on the examples in our swagger spec
  • a helpers/workflow directory that provides code that helps glue the pieces together for specific tasks such as adding a template.

Thanks for your continued support!

@mbernier
Copy link
Contributor

@Jericho - DUDE!!! This is awesome.

@mbernier
Copy link
Contributor

All - RE: Elmer's message:
We are hoping that any work that has been done to simplify and clean up the library, through the meaningful method names and other work, would still be VERY useful to the library - as we can just replace the client that you're using with the new one, within the methods.

This allows us to continue the work you're doing, while merging it with our own -- after we have your feedback on the client, of course!

@thinkingserious
Copy link
Contributor

Hello everyone, the new version of this library is available in beta here: https://github.com/sendgrid/sendgrid-csharp/tree/v3beta

I'm leaving this ticket open for evaluation against the new library.

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: help wanted requesting help from the community labels May 19, 2016
@steve-tapley
Copy link

Hi,
Can you let us know when this V3 will be available? At the moment, I'm having issues with SendGrid because of the bug in your async code.

The workaround described in the readme

transportWeb.DeliverAsync(emailMessage).Wait();

isn't a particularly great solution either.

Can I suggest that if the V3 is some time off, then this bug gets some attention?

Thanks,
Steve

@thinkingserious
Copy link
Contributor

Hello @steve-tapley,

We are a few weeks away from leaving beta and we have shifted all our open source resources to support the v3beta branch and the new v3 /mail/send endpoint: https://sendgrid.com/docs/API_Reference/Web_API_v3/Mail/index.html

I don't anticipate any major changes to the v3beta branch before launch, mostly the changes will be under the hood. The biggest change will be moving the endpoint from /mail/send/beta to /mail/send.

With Best Regards,

Elmer

@thinkingserious
Copy link
Contributor

@roryprimrose @samrueby @Jericho @steve-tapley This issue is now fixed, per: #235

The same offer regarding swag we made on issue 255 applies to all of you here. Especially @roryprimrose for providing the solution and @Jericho for implementing! :)

Now that we are out of beta, we are going to be looking at the next milestone for this library. We have much to do and we need help with prioritizing. If there are any features/issues that you want given attention, please open an issue or +1 existing issues. We will read and consider them all.

Currently, we plan to make the enhancements through Helpers: https://github.com/sendgrid/sendgrid-csharp/tree/master/SendGrid/SendGrid/Helpers and Mail was the first example. The next goals (tentative) for that Helper is to add library side validation, nice error handling and helper functions to mask away complexity and make common use cases easy. We are thinking the next area of Helpers might be templates, contactdb and perhaps subuser management.

Thanks for your continued support! All feedback is welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

6 participants