-
Notifications
You must be signed in to change notification settings - Fork 156
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
Scoped interface to disallow nested calls to onEnterScope/onExitScope. #178
Comments
I agree, but I'd argue against the relaxed option. I can't think of a single time we've one it on purpose. |
Relaxed option would provide a migration path toward Strict checking for existing code bases, though. |
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. |
Yes. |
@rjrjr - a follow-up question, do you think it should ignore double-registrations in the same scope? or, be strict about that as well? |
@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. |
Double-registrations are explicitly allowed, and I'm pretty sure that Presenters, for example, rely on this in their implementation.
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. |
@rjrjr pushed an update ^^^ with feedback addressed. |
should be it ^^^ |
…ed_calls Issue #178: Scoped interface to not allow nested onEnterScope/onExitScope calls.
When a
Scoped
object is registered in 2+ scopes (either nested or overlapping), the sequence of calls is:onEnterScope(A)
onEnterScope(B)
onExitScope()
<-- either B (nested) or A (overlapping)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 theScoped
interface doesn't explicitly address this situation, the burden falls on the implementor ofScoped
to guard against incorrect usage.I believe, there are two strategies to resolve this problem:
Scoped
object to be registered in multiple scopes (at a point in time).Scoped
object to be registered in multiple scopes, but call only firstonEnterScope
, and lastonExitScope
- 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. noscopedSubscribe
).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 toonEnterScope
andonExitScope
- since it's a guaranteed source of bugs.The text was updated successfully, but these errors were encountered: