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

1051 async OnEventAppeared #1310

Merged
merged 1 commit into from May 26, 2017

Conversation

5 participants
@rodolfograve
Contributor

rodolfograve commented May 22, 2017

Following from the discussions on #1051, this is a PR to support async OnEventAppeared.

This probably doesn't solve all the problems described in all the related issues (#861, #1051, #1179) but should at least allow real async OnEventAppeared handlers, i.e. give the client the ability to use async APIs without blocking at any point.

I have checked that all existing tests pass, and that our own real life applications runs correctly using this modified version.

API breaking changes:

  1. OnEventAppeared is now a Func<T, ResolvedEvent, Task>
  2. CatchUpSubscription.Start is now StartAsync

Please, let me know if there are improvements you require in order to get this PR approved.

Further improvements, which I'd be happy to work on as separate PRs if you'd be willing to consider them:

  1. Replace more of the existing TaskCompletionSource-based and manual ThreadPool with async/await. Taking this all the way down to the SocketAsyncEventArgs would help simplify the code, leaving a lot of the current complexity to the language/compiler by means of async/await
  2. The above should also help with scalability in cases where GetEventStore is sharing the Thread Pool with other components (OWIN, SignalR, other EventStore connections, etc.)
  3. Modernizing the code base to use more C#6 features like auto-implemented properties, coalesce and conditional operators, expression bodied functions, etc. This would help with readability. I understand why the team wouldn't want to spend some time on this (no clear value added) but perhaps the value proposition is different if an external person does it.
@gregoryyoung

This comment has been minimized.

Show comment
Hide comment
@gregoryyoung

gregoryyoung May 22, 2017

Member
Member

gregoryyoung commented May 22, 2017

@rodolfograve

This comment has been minimized.

Show comment
Hide comment
@rodolfograve

rodolfograve May 22, 2017

Contributor

@gregoryyoung I believe so. Perhaps something like this:

  1. Add an overload that accepts the original Action<EventStoreCatchUpSubscription, ResolvedEvent> and converts it into a Func<EventStoreCatchUpSubscription, ResolvedEvent, Task>:
() =>
{
  action();
  return Task.CompletedTask;
}

This solution would be backwards compatible and wouldn't add much extra cost for people that use Action, since Task.CompletedTask doesn't do any allocation and awaiting a completed task has a very small CPU cost.

  1. Just obsolete the current .Start and add the new StartAsync

Let me know if this would be acceptable to you.

PS. I'm also looking into the failed build. I remember reading somewhere that you no longer wanted to remain compatible with Mono. Can you please let me know if the build-mono4 is required to pass?

Contributor

rodolfograve commented May 22, 2017

@gregoryyoung I believe so. Perhaps something like this:

  1. Add an overload that accepts the original Action<EventStoreCatchUpSubscription, ResolvedEvent> and converts it into a Func<EventStoreCatchUpSubscription, ResolvedEvent, Task>:
() =>
{
  action();
  return Task.CompletedTask;
}

This solution would be backwards compatible and wouldn't add much extra cost for people that use Action, since Task.CompletedTask doesn't do any allocation and awaiting a completed task has a very small CPU cost.

  1. Just obsolete the current .Start and add the new StartAsync

Let me know if this would be acceptable to you.

PS. I'm also looking into the failed build. I remember reading somewhere that you no longer wanted to remain compatible with Mono. Can you please let me know if the build-mono4 is required to pass?

@rodolfograve

This comment has been minimized.

Show comment
Hide comment
@rodolfograve

rodolfograve May 22, 2017

Contributor

Apologies for the messy commits. Somehow I managed to merge my first failed issue1051-async-client-api-improvements branch into the "good" one "1051-async-eventAppeared". On the failed one I had applied some unnecessary changes that are now breaking the build.

Contributor

rodolfograve commented May 22, 2017

Apologies for the messy commits. Somehow I managed to merge my first failed issue1051-async-client-api-improvements branch into the "good" one "1051-async-eventAppeared". On the failed one I had applied some unnecessary changes that are now breaking the build.

@hayley-jean

This comment has been minimized.

Show comment
Hide comment
@hayley-jean

hayley-jean May 23, 2017

Contributor

Thanks for the PR @rodolfograve

We still support mono, we just no longer support mono3, so build-mono4 does still need to pass.
It looks like it's now the same tests failing on both builds, though, so fixing one should fix the other.

Just in case you weren't aware, you are able to run these same tests locally by running the scripts run_tests.cmd or run_tests.sh

Contributor

hayley-jean commented May 23, 2017

Thanks for the PR @rodolfograve

We still support mono, we just no longer support mono3, so build-mono4 does still need to pass.
It looks like it's now the same tests failing on both builds, though, so fixing one should fix the other.

Just in case you weren't aware, you are able to run these same tests locally by running the scripts run_tests.cmd or run_tests.sh

@rodolfograve

This comment has been minimized.

Show comment
Hide comment
@rodolfograve

rodolfograve May 23, 2017

Contributor

Ah, great @hayley-jean Between not being able to build using the same version of the compiler, and the tests, I was getting a bit frustrated at the very long feedback loop with this :-)

Thanks!

Contributor

rodolfograve commented May 23, 2017

Ah, great @hayley-jean Between not being able to build using the same version of the compiler, and the tests, I was getting a bit frustrated at the very long feedback loop with this :-)

Thanks!

@pgermishuys

This comment has been minimized.

Show comment
Hide comment
@pgermishuys

pgermishuys May 23, 2017

Member

@rodolfograve after looking through your commits this morning, it would appear that the appveyor build command was missing the SpecificVisualStudioVersion flag which has now been updated.

Apologies for the frustration this has caused you.

Member

pgermishuys commented May 23, 2017

@rodolfograve after looking through your commits this morning, it would appear that the appveyor build command was missing the SpecificVisualStudioVersion flag which has now been updated.

Apologies for the frustration this has caused you.

@rodolfograve

This comment has been minimized.

Show comment
Hide comment
@rodolfograve
Contributor

rodolfograve commented May 23, 2017

Thanks @pgermishuys

@pgermishuys

Just a quick scan over the PR which requires a detailed look through I just made some comments on things that can be removed.

@pgermishuys

Hey @rodolfograve, thanks for the PR!

We have to make sure that we aren't breaking the API, at least in this version.

You did provide the backwards support for the SubscribeToStreamAsync call but it would still break for users who are using any of the other API bits that previously used Action<Sub, ResolvedEvent, Task>.

@rodolfograve

This comment has been minimized.

Show comment
Hide comment
@rodolfograve

rodolfograve May 25, 2017

Contributor

Yes, my mistake. I added the missing methods. As an explanation for other readers, I chose to add the methods as extension methods on the interface (IEventStoreConnectionExtensions) to avoid forcing all implementers of the interface to have to provide the same implementation.

I have used this pattern in the past whenever a function can be defined in terms of other methods in the interface.

IEventStoreConnection.ConnectToPersistentSubscription is a good candidate, as it's implemented as ConnectToPersistentSubscriptionAsync(...).Wait()
Contributor

rodolfograve commented May 25, 2017

Yes, my mistake. I added the missing methods. As an explanation for other readers, I chose to add the methods as extension methods on the interface (IEventStoreConnectionExtensions) to avoid forcing all implementers of the interface to have to provide the same implementation.

I have used this pattern in the past whenever a function can be defined in terms of other methods in the interface.

IEventStoreConnection.ConnectToPersistentSubscription is a good candidate, as it's implemented as ConnectToPersistentSubscriptionAsync(...).Wait()
@pgermishuys

This comment has been minimized.

Show comment
Hide comment
@pgermishuys

pgermishuys May 25, 2017

Member

Thanks for making those changes @rodolfograve.
re-reviewing.

Member

pgermishuys commented May 25, 2017

Thanks for making those changes @rodolfograve.
re-reviewing.

@pgermishuys pgermishuys merged commit 16a1b4b into EventStore:release-v4.0.2 May 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
wercker/build-mono4 Wercker pipeline passed
Details
@@ -10,7 +10,7 @@ internal class EmbeddedEventStorePersistentSubscription : EventStorePersistentSu
internal EmbeddedEventStorePersistentSubscription(
string subscriptionId, string streamId,
Action<EventStorePersistentSubscriptionBase, ResolvedEvent> eventAppeared,
Func<EventStorePersistentSubscriptionBase, ResolvedEvent, Task> eventAppeared,

This comment has been minimized.

@damianh

damianh May 30, 2017

I think you should consider passing in a cancellation token
Func<EventStorePersistentSubscriptionBase, ResolvedEvent, CancellationToken, Task>

That way if a subscription is disposed (/system shutting down), eventAppeared handlers can participate in co-operative cancellation. Usually that means passing the token to something else that does async I/O and is cancellable.

This is the signature I used https://github.com/SQLStreamStore/SQLStreamStore/blob/master/src/SqlStreamStore/Subscriptions/StreamMessageReceived.cs#L20

Also something that may or may not be useful, my TaskQueue implementation.

@damianh

damianh May 30, 2017

I think you should consider passing in a cancellation token
Func<EventStorePersistentSubscriptionBase, ResolvedEvent, CancellationToken, Task>

That way if a subscription is disposed (/system shutting down), eventAppeared handlers can participate in co-operative cancellation. Usually that means passing the token to something else that does async I/O and is cancellable.

This is the signature I used https://github.com/SQLStreamStore/SQLStreamStore/blob/master/src/SqlStreamStore/Subscriptions/StreamMessageReceived.cs#L20

Also something that may or may not be useful, my TaskQueue implementation.

This comment has been minimized.

@rodolfograve

rodolfograve Jun 2, 2017

Contributor

That is absolutely an improvement I would like to see.

Unfortunately it is not required for our use case and I have no more day time to spend on this. If no one else does it, I'll try to add this in as a separate PR.

@rodolfograve

rodolfograve Jun 2, 2017

Contributor

That is absolutely an improvement I would like to see.

Unfortunately it is not required for our use case and I have no more day time to spend on this. If no one else does it, I'll try to add this in as a separate PR.

pgermishuys added a commit that referenced this pull request Jun 6, 2017

hayley-jean added a commit that referenced this pull request Jul 31, 2017

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