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

Add Synchronous to MySqlConnectionStringBuilder. #21

Closed
wants to merge 2 commits into from

Conversation

ejball
Copy link
Contributor

@ejball ejball commented Jul 11, 2016

This initial implementation could have adverse affects on performance, so you might not want to merge, but it is an idea for enforcing synchronous access, even when using the async methods, to improve debugging.

if (!Synchronous)
return task;

return new ValueTask<T>(task.AsTask().GetAwaiter().GetResult());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have an optimised code path for the case where the ValueTask already has a value (which is expected to be common, hence the use of ValueTask)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe .AsTask() returns a completed task efficiently; but still there is unnecessary allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be a great change.

@bgrainger
Copy link
Member

I'm not convinced I like this approach. I haven't attempted it, but it feels like one could make the socket operations synchronous, then use ValueTask to bubble the results as far up the call stack as possible. (Then, the remaining uses of Task would still be synchronous.)

This ties into an improvement I want to make to use all new non-public methods that return ValueTask in ExecuteDbDataReaderAsync and reimplement the public API in terms of those new ValueTask methods.

@bgrainger bgrainger force-pushed the master branch 5 times, most recently from eb9bbeb to 7754ffa Compare July 11, 2016 02:53
@ejball
Copy link
Contributor Author

ejball commented Jul 11, 2016

I don't like it either, but I wasn't ready to do the hard work of using synchronous socket operations. This is just a proof of concept that may encourage someone to eventually do it right. :-)

Setting Synchronous to true forces async requests to block on the calling thread to improve debugging.
@ejball
Copy link
Contributor Author

ejball commented Jul 11, 2016

Rebased.

@bgrainger
Copy link
Member

wasn't ready to do the hard work of using synchronous socket operations

Turned out to not be that hard; code is referenced from #62. I'm going to close this PR in favour of (the eventual resolution of) that one.

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

Successfully merging this pull request may close these issues.

2 participants