"bindFragment" Method Signature

[JakeWharton]

It takes Object and uses ugly instanceof and runtime API checks. Why not just have two overloads?

[mttkay]

Because that would crash your app if you're not using the support library,
or if you're using it and are on a Gingerbread device. The class loader
needs to resolve these symbols if they're part of the method signature. I'm
open for suggestions though for how to make this nicer.
On Nov 27, 2014 7:26 AM, "Jake Wharton" [email protected] wrote:

It takes Object and uses ugly instanceof and runtime API checks. Why not
just have two overloads?

—
Reply to this email directly or view it on GitHub
#95.

[JakeWharton]

That's not true since Donut.
On Nov 26, 2014 11:24 PM, "Matthias Käppler" [email protected]
wrote:

Because that would crash your app if you're not using the support library,
or if you're using it and are on a Gingerbread device. The class loader
needs to resolve these symbols if they're part of the method signature.
I'm
open for suggestions though for how to make this nicer.
On Nov 27, 2014 7:26 AM, "Jake Wharton" [email protected] wrote:

It takes Object and uses ugly instanceof and runtime API checks. Why not
just have two overloads?

—
Reply to this email directly or view it on GitHub
#95.

—
Reply to this email directly or view it on GitHub
#95 (comment).

[mttkay]

What is not true and how? This has nothing to do with Android (or Donut for
that matter), if you're loading a class at runtime that is referencing
symbols that are missing at runtime, the run time will throw
NoClassDefFound. It's a runtime linker problem, since we compile with
provided scope, so only the symbol references survive, but not the classes
that are referenced. If you then don't bundle the support library with your
app, it'll crash.
On Nov 27, 2014 9:11 AM, "Jake Wharton" [email protected] wrote:

That's not true since Donut.
On Nov 26, 2014 11:24 PM, "Matthias Käppler" [email protected]
wrote:

Because that would crash your app if you're not using the support
library,
or if you're using it and are on a Gingerbread device. The class loader
needs to resolve these symbols if they're part of the method signature.
I'm
open for suggestions though for how to make this nicer.
On Nov 27, 2014 7:26 AM, "Jake Wharton" [email protected]
wrote:

It takes Object and uses ugly instanceof and runtime API checks. Why
not
just have two overloads?

—
Reply to this email directly or view it on GitHub
#95.

—
Reply to this email directly or view it on GitHub
#95 (comment).

—
Reply to this email directly or view it on GitHub
#95 (comment).

[JakeWharton]

It won't. We do this all the time. It only crashes when you reflect on the class' method list (which nobody will be doing).

[JakeWharton]

Here's an example:

In Okio we include overloads for Okio.source which take either a File (for Android & old and busted Java) as well as Path & OpenOption (for new hotness Java). Even on APIs 19 and 21 Android's Java 7 implementation is incomplete, and java.nio.file.Path and java.nio.file.OpenOption are not present. However, Okio.source calls work just fine because you can never link a method call site against the version that declares Path. Donut contained the last classloader which eagerly verified and enforced the presence of all types in method parameters and return types.

public static Source source(File file) { ... }
public static Source source(Path path, OpenOption... options) { ... }

https://github.com/square/okio/blob/126258a88cacf2d2a0186ac613dc8142e37641a0/okio/src/main/java/okio/Okio.java#L165-L176

[mttkay]

There's a difference between method symbols and class symbols. If I follow
your example (please correct me if I'm wrong), you're saying that the
methods defined on said class are different across different versions of
that class, but the class itself always exists. I agree, this will not
crash. The reason it doesn't crash is because that code (the method) is
only seen by the runtime when you're trying to invoke it, that is, the
method handle is on your current instruction stack.

That is different from what the class loader does. The class loader will
throw if the entire class (not method) you're referencing is nowhere to be
found in the chain of class loaders in scope. Unless I'm totally missing
something about how Java works, this cannot work. And I'm pretty confident
I did write this code for the very reason that it crashed.

Again, feel free though to revert that change and test it out. I'm more
than happy to get rid of that ugliness that is Object parameters if there's
any chance to do that.
On Nov 27, 2014 9:53 AM, "Jake Wharton" [email protected] wrote:

Here's an example:

In Okio we include overloads for Okio.source which take either a File
(for Android & old and busted Java) as well as Path & OpenOption (for new
hotness Java). Even on APIs 19 and 21 Android's Java 7 implementation is
incomplete, and java.nio.file.Path and java.nio.file.OpenOption are not
present. However, Okio.source calls work just fine because you can never
link a method call site against the version that declares Path. Donut
contained the last classloader which eagerly verified and enforced the
presence of all types in method parameters and return types.

public static Source source(File file) { ... }public static Source source(Path path, OpenOption... options) { ... }

https://github.com/square/okio/blob/126258a88cacf2d2a0186ac613dc8142e37641a0/okio/src/main/java/okio/Okio.java#L165-L176

—
Reply to this email directly or view it on GitHub
#95 (comment).

[JakeWharton]

That was why I referenced Donut's behavior. It was the last class loader which actually did throw in this case (method parameter type or method return type not present). It's why when you look in the support-v4 library, for example, you see all these stupid indirections to conditionally load API-specific implementation classes. In Eclair and after it is either completely silent or just writes a log line.

I can get a test of it going at some point (or someone else watching can).

[mttkay]

That I was totally unaware of. That's interesting indeed. Would love to see
a test around this. I did write this code long after Donut (tested against
Gingerbread most likely), so I'm a little confused as to what it was that
caused the crash. Burn it with fire then.
On Nov 27, 2014 10:27 AM, "Jake Wharton" [email protected] wrote:

That was why I referenced Donut's behavior. It was the last class loader
which actually did throw in this case (method parameter type or method
return type not present). It's why when you look in the support-v4 library,
for example, you see all these stupid indirections to conditionally load
API-specific implementation classes. In Eclair and after it is either
completely silent or just writes a log line.

I can get a test of it going at some point (or someone else watching can).

—
Reply to this email directly or view it on GitHub
#95 (comment).

[dlew]

Honestly, what's even the point of bindFragment() and bindActivity() (if we're adding unsubscriptions to the lifecycle)? While they protect you from emitting items after pause, they don't auto-unsubscribe unless an item is emitted, which practically speaking means you have to manage the subscription yourself unless you want a possible memory leak.

[JakeWharton]

¯\_(ツ)_/¯

Never used it. Your reasoning certainly seems sounds. My only concern was from a pure API standpoint.

[hamidp]

We probably can just start off with deprecating those two (in a separate PR) and going from there.

[mttkay]

See my comments in the PR. These methods are safe guards against a race
condition with message order in the main looper. They have nothing to do
with the intention to auto unsubscribe, rather than catching rogue
notifications that arrive in your subscriber after your fragment view has
been destroyed
On Nov 28, 2014 6:27 PM, "Hamid" [email protected] wrote:

We probably can just start off with deprecating those two (in a separate
PR) and going from there.

—
Reply to this email directly or view it on GitHub
#95 (comment).

[mttkay]

@dlew continuing the discussion of #105 here.

The original discussions are here:

ReactiveX/RxJava#1292
ReactiveX/RxJava#1407
ReactiveX/RxJava#1590

There were also multiple discussions (and PRs) for adding variants of HandlerThreadScheduler that schedules work synchronously if the caller is already on the main thread. They were rejected for multiple reasons in the past, partly because they were optimizations based on hunches or flawed micro benchmarks, but more importantly IMHO because it breaks the contract of HandlerThreadScheduler, which is to predictably schedule work on the main looper. If you weaken that contract you might run into surprising and difficult to understand situations where suddenly the main looper is bypassed and messages/notifications get processed in an unpredictable order, so I was against landing these changes.

I do see though how it's also causing trouble at the same time, so finding some middle ground, e.g. by making this change very explicit would be nice. Relevant: #3

[JakeWharton]

Ok, the way it stands now this will actually fail at compilation time, not runtime (see #136).

This is because the methods become "pure" overloads of each other. That is, they have the same name, return type, number of parameters, and type of second parameter. They only differ by the type of the first parameter.

In the very few times I've done this, the overloads have differed in some way even if that difference produces the same API at the call site. A prime example: varargs.

Changing the support-v4 variant's method signature to the following:

bindFragment(android.support.v4.app.Fragment fragment, Observable<T> source, Object... dummy)

actually works! You can also rename the method to bindSupportFragment and it will work. Neither approach fails at runtime.

So, which do we like?

[rosshambrick]

A little late for a reply I guess, but I'd prefer bindFragment(...) for two reasons. 1- It wouldn't be a breaking change and 2 - the addition of 'Support' doesn't add much value. This is assuming it technically works just as well either way.

[JakeWharton]

Per my comment above, it does not work.

[rosshambrick]

Ah yes, I missed Object... dummy the first time somehow. Carry on