Skip to content
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

Do not specify ThreadAsserter globally #119

Open
Jeffset opened this issue Dec 29, 2023 · 1 comment
Open

Do not specify ThreadAsserter globally #119

Jeffset opened this issue Dec 29, 2023 · 1 comment
Labels
api Related to introducing new API or changing existing one
Milestone

Comments

@Jeffset
Copy link
Collaborator

Jeffset commented Dec 29, 2023

Now, Yatagan.threadAsserter property (Yatagan.setThreadAsserter() method for 1.x.y) sets thread asserted globally. This is a poor design choice considering a project may depend on libraries or frameworks, that use Yatagan internally; as all the graphs will share the same global Yatagan state, setting a thread asserter globally is intrusive to all the other graphs and may disrupt their behavior.

Here I propose to introduce a new API for 2.0 that will not have these issues - the thread asserter must be specified explicitly for every graph, not globally.

@Jeffset Jeffset added the api Related to introducing new API or changing existing one label Dec 29, 2023
@Jeffset Jeffset added this to the 2.0.0 milestone Dec 29, 2023
@Jeffset
Copy link
Collaborator Author

Jeffset commented Dec 29, 2023

Let's try to go over sensible options on how to implement this:

1. Binding approach.

The graph just uses the object bound to com.yandex.yatagan.ThreadAsserter.

class MyTAImpl @Inject constuctor() : ThreadAsserter { ... }
// ...
/** Magic binding */
@Binds bindThreadAsserter(i: MyTAImpl): ThreadAsserter

Pros:

  • Kinda natural, just a special binding, no need to introduce and support ad-hoc APIs just for this.
  • Potentially great flexibility on how to provide an asserter.

Cons:

  • All bunch of things that come from the fact, that ThreadAsserter is just a regular binding:
    • Can it have dependencies? In general, no - it can't, because thread asserter is an implicit dependency of every scoped binding in the graph.
    • Can it be scoped? If it's unscoped, it's likely expensive to create an object every time just to assert a thread. If it is scoped, then it's still an additional overhead to do a single-check* every time.
  • This flexibility may turn into additional complexity and some unintuitive behaviors, that are unneeded for such a simple thing.
  • This kind of optional binding doesn't work well with AutoBuilder: We can know if the asserter really is present only at runtime. This inhibits some codegen optimizations.

2. Separate API.

This may be in the form @Component(..., threadAsserter = MyTAImpl::class) or a separate annotation @ComponentThreadAsserter(MyTAImpl::class).

class MyTAImpl() : ThreadAsserter { ... }
// ...
@Component(..., threadAsserter = MyTAImpl::class)
// ...

Pros:

  • It's known at compile time, whether the asserter is present or not.
  • It doesn't have any additional complexity and unneeded flexibility.
  • It may be explicitly stated, that the ThreadAsserter instances are created in root component's constructor and stored in private final field for efficiency.

Cons:

  • This way restricts the way how and when ThreadAsserter instances are created. Implementations must have a public default parameterless constructor (or be a Kotlin-like Object?).

3. Mixed approach.

We introduce a special binding type that is module-hosted, e.g. @ProvideThreadAsserter. It is not allowed to have neither a scope nor any dependencies. It's explicitly stated, that the method is invoked once in component's constructor and the result is saved and used where required.

@Module
class MyThreadAsserterModule(
  @get:ProvideThreadAsserter
  val asserter: MyTAImpl,
)
//...
@Component(..., modules = [..., MyThreadAsserterModule::class])
//...

Pros:

  • All the pros of the above, I guess
  • With Module-based approach it can be chosen to provide a global singleton, or to provide a local object with module instance.

Cons:

  • None of the above, I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to introducing new API or changing existing one
Projects
None yet
Development

No branches or pull requests

1 participant