Scoped interface to disallow nested calls to onEnterScope/onExitScope.

[valeriyo]

When a Scoped object is registered in 2+ scopes (either nested or overlapping), the sequence of calls is:

1. onEnterScope(A)
2. onEnterScope(B)
3. onExitScope() <-- either B (nested) or A (overlapping)
4. onExitScope() <-- either A (nested) or B (overlapping)

For the implementer of the Scoped interface, this becomes a challenge, because the most straightforward implementation will re-initialize the object in step 2 (possibly leaking resources created in step 1), and release resources in step 3 (possibly leading to crashes before step 4). Since the Scoped interface doesn't explicitly address this situation, the burden falls on the implementor of Scoped to guard against incorrect usage.

I believe, there are two strategies to resolve this problem:

• "Strict" -- do not allow a Scoped object to be registered in multiple scopes (at a point in time).
• "Relaxed" -- allow Scoped object to be registered in multiple scopes, but call only first onEnterScope, and last onExitScope - so that the object doesn't need to track its scope count.

NOTE: the "Relaxed" strategy works only if the implementation doesn't tie any of its resources to the scope passed as parameter to onEnterScope (e.g. no scopedSubscribe).

What I propose is a mix of the two strategies: "Strict" by default, and "Relaxed" for classes that use a special annotation.

In either case, the Scoped interface needs to disallow nested calls to onEnterScope and onExitScope - since it's a guaranteed source of bugs.

[rjrjr]

I agree, but I'd argue against the relaxed option. I can't think of a single time we've one it on purpose.

[valeriyo]

Relaxed option would provide a migration path toward Strict checking for existing code bases, though.

[rjrjr]

How about if we mark it deprecated?

Meh, no, I really think we should just pull the band-aid off. See discussion in MortarScope in the PR.

[valeriyo]

@rjrjr - to summarize your comments in https://github.com/square/mortar/commit/233c143a22095cd232bbe79885e446f11bda3cc8 - you think that MultiScoped shouldn't exist, and the "Strict" strategy enforced (without a static map, somehow)?

[rjrjr]

Yes.

[valeriyo]

@rjrjr - a follow-up question, do you think it should ignore double-registrations in the same scope? or, be strict about that as well?

[valeriyo]

@rjrjr - regarding "Strict" vs "Relaxed" approaches, there must be situations where "ref-counting" approach is preferable, vs. having to always figure out the proper "owner" scope for each object.

[rjrjr]

Double-registrations are explicitly allowed, and I'm pretty sure that Presenters, for example, rely on this in their implementation.

  /**
   * Register the given {@link Scoped} instance to have its {@link Scoped#onEnterScope(MortarScope)}
   * and {@link Scoped#onExitScope()} methods called. Redundant registrations are safe,
   * they will not lead to additional calls to these two methods.
   * <p>
   * {@link Scoped#onEnterScope(MortarScope) onEnterScope} is called synchronously.
   *
   * @throws IllegalStateException if this scope has been destroyed
   */
  public void register(Scoped scoped) {

Re: "there must be situations," we don't build features in speculatively. Every new feature is a maintenance burden and a drag on other development. Paying that price because someone probably wants it just doesn't make sense. If we don't have at least three use cases in hand already that will use the feature, it should not be built.

Even then, if a use case can reasonably be handled by user code written on top of the library, it should not be built into the library. Each ounce of "convenience" added to the library dramatically increases its maintenance burden. The bar for adding features needs to be very high. So if we did find a case where double-registration was desirable, I'd still be very opposed to building in ref-counting.

But I'm deeply sceptical that there is anything we can do with relaxed, double registration that can't also be accomplished with multiple registration in a strict world.

[valeriyo]

@rjrjr pushed an update ^^^ with feedback addressed.

[valeriyo]

should be it ^^^