Skip to content

utility function for creating observables for closeable resources #1282

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

Merged
merged 2 commits into from
Jul 8, 2014

Conversation

abersnaze
Copy link
Contributor

#1199 the answer to the issue was nagging at me when I tried to recommend it recently. I think @zsxwing's solution was ok but would fail if there were repeated subscriptions to the observable returned. I created a utility function on StringObservable to make it easier to use using() for objects that implement the AutoCloseable interface.

PS. I made a breaking change to arguments of join(Observable<Object>) to join(Observable<String>) and added a Observable<String> toString(Observable<Object>) to make up for it.

PPS. I made a UnsafeFunc0<R> scoped to this class but I would like to have UnsafeFunc<N> in the rx.functions packages and have Func<N> extends UnsafeFunc<N>. Makes it clear to the compiler that we don't expect the code the user's are giving us to always work. This also helps the users because they don't have to always fill their closures with try catch blocks

() -> new FileReader(file)

vs.

() -> {
    try {
        return new FileReader(file);
    } catch (Exception e) {
        throw new RuntimeException(e);
    }
}

@cloudbees-pull-request-builder

RxJava-pull-requests #1182 SUCCESS
This pull request looks good

@@ -50,6 +54,73 @@
public static Observable<byte[]> from(final InputStream i) {
return from(i, 8 * 1024);
}

private static class CloseableResource<S extends AutoCloseable> implements Subscription {
Copy link
Member

Choose a reason for hiding this comment

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

This won't work with Java 6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I'll switch it to Closeable then

@cloudbees-pull-request-builder

RxJava-pull-requests #1186 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Member

Can someone else involved in StringObservable review this please?

Also, this needs to be rebased now as it collides with another change that was merged.

@cloudbees-pull-request-builder

RxJava-pull-requests #1191 FAILURE
Looks like there's a problem with this pull request

UnsafeFunc0 now extends Callable.
@abersnaze
Copy link
Contributor Author

whoops bad merge

@cloudbees-pull-request-builder

RxJava-pull-requests #1192 SUCCESS
This pull request looks good

@abersnaze
Copy link
Contributor Author

@davidmoten can you take a look?

@davidmoten
Copy link
Collaborator

yep will do

* @param <R>
*/
public static interface UnsafeFunc0<R> extends Callable<R> {
public R call() throws Exception;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might have missed something here. I don't see that the UnsafeFunc0 adds much. After all every Func0 is potentially unsafe inasmuch as it can throw a RuntimeException for instance.

@davidmoten
Copy link
Collaborator

I see your point about the advantage when writing lambdas. Could you equivalently have a utility function that wrapped the lambda to do the wrapping with a RuntimeException as in:

toRuntimeException( () -> new FileReader(file))

If it works you might get your code brevity back without your changes being necessary.

@davidmoten
Copy link
Collaborator

ah I just looked up java 8 lambdas and checked exceptions and I see my suggestion has no legs

@davidmoten
Copy link
Collaborator

Righto so I've tested the handling of lambdas and checked exceptions in java 8 and I can see this is a handy utility. It's a really interesting idea to then extend FuncN from UnsafeFuncN and I can see this would work. The more extreme implementation would be to add throws Exception to the definition of FuncN. Perhaps the enforced try catch in writing Operators that call FuncN will be worth the abbreviation for rxjava users. What would steer us away from this?

My only concern then is really the name because Funcs are inherently unsafe as there is nothing in the contract for a Func that says it can't throw a RuntimeException or Error. The alternative is something yuk like FuncCanThrowCheckedException.

@abersnaze
Copy link
Contributor Author

How about CheckedFunc? I agree that I would like to see CheckedFunc used in core where ever an RuntimeException from a Func is caught and pushed down to onError. The reasoning is that if the implementation is already wrapping calls with a try/catch then let the Func throw whatever it wants.

@davidmoten
Copy link
Collaborator

Do you see an impediment to changing the signature of FuncN to include throws Exception?

@abersnaze
Copy link
Contributor Author

Adding to FuncN directly would be too much. It forces operator implementations to always wrap exceptions even if they don't care about propagating RuntimeExceptions up the call stack.

@davidmoten
Copy link
Collaborator

there's the tradeoff I suppose between operator implentations and users using java 8 lambdas with rx operators and catching exceptions. In theory there'd be more of the latter right?

@davidmoten
Copy link
Collaborator

I'm thinking about Observable.map() in particular and the plethora of java methods that throw checked exceptions. Would be nice to make lambda usage cleaner.

@abersnaze
Copy link
Contributor Author

The map operator explicit does a try catch around the Func1 call so that would be a candidate for switching to CheckedFunc1.

@davidmoten
Copy link
Collaborator

if we did map() I'd be tempted to do it everywhere. Might have to do an inventory of where Func is used in the Operators

@davidmoten davidmoten mentioned this pull request Jun 4, 2014
benjchristensen added a commit that referenced this pull request Jul 8, 2014
utility function for creating observables for closeable resources
@benjchristensen benjchristensen merged commit 032d3af into ReactiveX:master Jul 8, 2014
@abersnaze abersnaze deleted the closeable branch August 13, 2014 18:10
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.

5 participants