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

Subscription Utilities and Default Implementations #173

Closed
benjchristensen opened this Issue Mar 7, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@benjchristensen
Member

benjchristensen commented Mar 7, 2013

Provide a createSubscription() utility that returns a Subscription implementation that is thread-safe that provides an isUnsubscribed() getter to be used by most Observables via Observable.create() since most of the time they just need a thread-safe volatile/atomic boolean to check in a loop.

Also, where should it go along with these:

    public static Subscription createSubscription(final Action0 unsubscribe)
    public static Subscription createSubscription(final Object unsubscribe)
    public static Subscription noOpSubscription()

Should this be on Observable or should a new Subscriptions utility class exist?

@ghost ghost assigned benjchristensen Mar 7, 2013

@benjchristensen

This comment has been minimized.

Member

benjchristensen commented Mar 7, 2013

The type of things to model in Rx are here: http://msdn.microsoft.com/en-us/library/system.reactive.disposables(v=vs.103).aspx

Disposable == Subscription in RxJava.

The 'Disposable' name means something special in C# similar to AutoCloseable (http://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html) in Java7 (which is why Subscription doesn't implement AutoCloseable yet since we are sticking to Java6).

We want an rx.subscriptions package.

Subscription is currently an interface (which I like in this case since it can be a FunctionalInterface in Java8 and implemented with lambdas) but it means we can't exactly model the Subscription.create() method signature of Rx.Net.

I'm torn ... as Subscription.create() would be a very nice method signature.

We could have `rx.subscriptions.Subscriptions' as the utility method but it's slightly contrived.

For example:

    rx.subscriptions.Subscriptions.create(final Action0 unsubscribe)
    rx.subscriptions.Subscriptions.create(final Object unsubscribe)
    rx.subscriptions.Subscriptions.empty()

Then we'd have a class like `rx.subscriptions.BooleanSubscription' which is what would provide the functionality most Observables want.

@benjchristensen

This comment has been minimized.

Member

benjchristensen commented Mar 7, 2013

Anyone have thoughts on what decisions to make with this API?

This was never tackled internally at Netflix as we just had custom Subscription objects that implement the interface - but that's not appropriate for a standalone library, particularly when trying to comply with the Rx.Net design and API.

@mairbek

This comment has been minimized.

Contributor

mairbek commented Mar 7, 2013

I like the idea of creating Subscriptions class and moving it alongside with a Subscription interface to a separate package.

It seems like .NET naming convention dictates that interfaces should start with I and static class should be named as a singular word (IDisposable -> Disposable). Since we are dealing with Java, having Subscriptions as a utility class seems to be a natural choice because this naming style is widely used within a JDK itself, e.g. Collection -> Collections.

Besides that I see there are several wrappers in a System.Reactive.Disposables, some of them like CompositeDisposable seems to be particularly useful. It would be nice to see whole namespace migrated to RxJava.

@benjchristensen

This comment has been minimized.

Member

benjchristensen commented Mar 12, 2013

Done ... interested in people's review however. Will be part of 0.6.0 as it involves some small breaking changes in the pursuit of trying to match Rx.Net (such as this https://rx.codeplex.com/SourceControl/changeset/view/6bf8a7952bdd#Rx/NET/Source/System.Reactive.Core/Reactive/Disposables/Disposable.cs)

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