Warn when RequestHandler doesn't invoke response.onSubscribe()

[yschimke]

Can we detect this after requestHandler.handle*() is called and print a warning or fail noisily and early? At least during development this should be the default.

production behaviour to be discussed.

[ghost]

Hmm, can we implement a wrapper around RequestHandler, or better: a mixin, that checks this assertion? It might seem like a trivial check, but it increases compiled code size and is not something we want in prod, I suppose.

[benjchristensen]

it increases compiled code size

I'm struggling to see why this is a concern. I buy that it may not be a check we care about, but why is "compiled code size" a big deal here? And how much increase are we talking about?

[yschimke]

My concern with the mixin approach is that it relies on a n00b user doing extra work to get warned about n00b errors.

[ghost]

Code size is the most important on mobile, I think. It won't be a big change for this particular class, as it's used in a few spots only. I don't know how big deal it would be for this class, but my wild guess is that logging is responsible for as much code in Channel{Responder,Requester} as actual functionality. Most importantly, it's easy to avoid.

[ghost]

My concern with the mixin approach is that it relies on a n00b user doing extra work to get warned about n00b errors.

Good point. Let's go with wrapper then, as we can wrap any handler provided by a user into it, without knowing runtime type.

[benjchristensen]

Regarding logging, the entire RxJava library has no logging, so I'm quite alright with us avoiding what is generally just a waste of space.

We should support hooking in for insights when we want, but I don't like code littered with logging everywhere just because.

[benjchristensen]

Code size is the most important on mobile

Agreed. If it's trivial though we shouldn't prevent correctness/safety guards.

[ghost]

We should support hooking in for insights when we want, but I don't like code littered with logging everywhere just because.

Mixin/wrapper-based approach provides nice separation between logic and logging. Look at the amount of debug logging we already have and compare it with the number of LOG(...) statements in the code. I'd say it's quite good ratio :)

[lehecka]

I would suggest relaxing the original proposal to this:
after requestHandler.handle* is called we validate that
a) response.OnSubscribe was called
b) response.OnComplete/OnError was called

This will simplify writing the handler for the cases when the handler wants to terminate right away. The subscription object is unnecessary for sending the terminating signal.

[lehecka]

the most relaxed solution was implemented. onSubscribe doesn't have to be called immediately.
Closing the issue as the discussion is closed now.