Skip to content

Conversation

@akarnokd
Copy link
Collaborator

Inline the use of AnonymousObserver at various locations:

  • Subscribe with CancellationTokenSource
  • Remoting Subscribe.

Also make Notification.OnCompletedNotification static singleton; it is stateless so there is no reason to create an instance of it all the time.

/// to be disposed upon termination.
/// </summary>
/// <typeparam name="T">The element type of the sequence.</typeparam>
internal sealed class ObserverWithToken<T> : IObserver<T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you relate that to AnonymousSafeObserver and SafeObserver and evaluate whether we can save some code/inherit/reuse/etc. here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those have safeguards and need a disposable upfront. This is just relaying events and calling dispose on the resource.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but they are internal and might be reworked to not need the disposable upfront. That way, even the SingleAssignmentDisposable in Producer could be saved. The would just have to expose some interface, e.g. ISafeObserver

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can try that after this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright.

@danielcweber danielcweber merged commit 4fe9ad2 into dotnet:master Jun 14, 2018
@akarnokd akarnokd deleted the InlineAnonymousObserver branch June 26, 2018 21:34
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.

2 participants