Singleton behavior of CurrentThreadScheduler

[MichaelSchneeberger]

From #358 and #363 the following question arises:

1. Should CurrentThreadScheduler have a singleton behavior by default? Or, should the singleton behavior be exposed through a static method like:

scheduler = CurrentThreadScheduler.singleton()

2. if yes, why do we need the __new__ method in CurrentThreadScheduler? I have shown in Don't instantiate new CurrentThreadScheduler._Local with each scheduler instance #363 that the CurrentThreadScheduler also works without it. Removing it would eliminate the problem of extending CurrentThreadScheduler as explained in Enforce CurrentThreadScheduler to run on a dedicated thread? #358.

[erikkemperman]

I have no strong preference either way, and you’re right that removing __new__ would restore the 1.x behaviour and make subclassing work (although I believe can make the latter work with __new__ as well).

For the record, though, I only added the __new__ method to enforce the singleton as that’s how it is used internally, and I figured it might make sense to enforce it externally as well. Perhaps I’m wrong about that, though I think the scenario presented as a use case is maybe not something it was ever explicitly intended to support; it seems kind of an obscure means of synchronization and I feel there ought to be better ways of achieving that.

[MichaelSchneeberger]

removing __new__ would restore the 1.x behaviour

No, I don't claim that it would restore the 1.x behavior. I claim that removing the __new__ method would not change the behavior of the current CurrentThreadScheduler implementation at all. The __new__ method registers the CurrentThreadScheduler objects (which have no fields) in the mapping _global. Saving an object that has no fields, however, won't do much.

it seems kind of an obscure means of synchronization and I feel there ought to be better ways of achieving that.

You are probably referring to the factorial example from #358. I think that question is a more general rx question. If I get a cold rx observable (possibly from a third party library), should I be able to locally execute it with subscribe_on and integrate it into my rx stream with flat_map or not?

[erikkemperman]

No, I don't claim that it would restore the 1.x behavior.

Ok, sorry, I guess I claim it then :-)

Saving an object that has no fields, however, won't do much.

To be honest I don't see how whether or not it has fields come into play, the point of this method was only to enforce the singleton.

The subclass problem can be resolved while keeping this method, I'm pretty sure. But I think that would still leave your main issue unaddressed, right?

@dbrattli @jcafhe @MainRo

I would really like to see this issue resolved before the imminent v3 release... I'm happy either way, e.g. removing the __new__ method and keeping the singleton, or alternatively making the __new__ method play nice with inheritance and accept this (minor, imho) departure from 1.x behaviour with a note in the changelog. What do you think?

[MichaelSchneeberger]

Ok, let's prove then if removing the __new__ method will "restore the 1.x behaviour" or not. To do so, I will check if the pointer to PriorityQueue changes for each CurrentThreadScheduler object initialized in the main thread or not.

For RxPY v1.6 I get:

<rx.internal.priorityqueue.PriorityQueue object at 0x7f73f04e15c0>
<rx.internal.priorityqueue.PriorityQueue object at 0x7f73ee805a90>

For RxPY v3.0 I get:

<rx.internal.priorityqueue.PriorityQueue object at 0x7f3b8e90d0b8>
<rx.internal.priorityqueue.PriorityQueue object at 0x7f3b8e90d0b8>

And for RxPY v3.0, where the __new__ method has been simply commented out, I get:

<rx.internal.priorityqueue.PriorityQueue object at 0x7fbabcc42048>
<rx.internal.priorityqueue.PriorityQueue object at 0x7fbabcc42048>

As you can see, removing the __new__ method does not restore the 1.x behavior.

The method I used in RxPY v1.6 to check this is:

scheduler1 = CurrentThreadScheduler()


def action1(_, __):
    print(scheduler1.queue)
    scheduler2 = CurrentThreadScheduler()

    def action2(_, __):
        print(scheduler2.queue)

    scheduler2.schedule(action2)

scheduler1.schedule(action1)

And for RxPY v3.0, it is:

scheduler1 = CurrentThreadScheduler()

def action1(_, __):
    print(scheduler1._local.queue)
    scheduler2 = CurrentThreadScheduler()

    def action2(_, __):
        print(scheduler2._local.queue)

    scheduler2.schedule(action2)

scheduler1.schedule(action1)

In #363, I did a similar test, which involved more than one thread.

[erikkemperman]

All right, if you have several schedulers on the same thread, I concede the point. I think that goes slightly against the implied semantics of the scheduler's name, but you are right that's the way it used to be.

Sorry, we seem to be going in circles, probably due to my still misunderstanding exactly what changes you would like to see.

[jcafhe]

Here is what I've understood about CurrentThreadScheduler (sorry for the wall of text):

A Local instance declared as class member of CurrentThreadScheduler is instantiated at import time. As demonstrated above, it is fine because we need to contruct a Local only once to avoid overriding issues. The Local object will by itself dynamically create an instance per thread at access time.

Also, if I create an instance of CurrentThreadScheduler on thread A, and call its scheduling methods from thread B, no instance of CurrentThreadScheduler will be created. Of course, this will work since the underlying Local will take care of constructing a new Local bound to thread B. Then, if I create an CurrentThreadScheduler on thread A, I will get the previous instance if it's still referenced somewhere and has not already be garbage collected (not sure if gc is involved here), else a new one.

So I believe the policy here is that it exists zero or one referenced CurrentThreadScheduler per thread, and the number of Local instances does not match necessarily the number of referenced CurrentThreadScheduler instances.

My point here is if we keep the singleton pattern, we can make a true singleton based on __new__, i.e. only one instance of CurrentThreadScheduler, and let the ubiquitous Local class member take care of spawning a Local object per thread. I know that it breaks a little the semantic, but I believe it would not be effective anyway to instantiate a CurrentThreadScheduler each time we detect an access from another thread.

From an rx1.6 end-user perspective, I was really confused about what should I use, current_thread_scheduler or CurrentThreadScheduler()? Can I re-use an instance of CurrentThreadScheduler that I previously and explicitly created?

So I guess whether the singleton or the 'multi-instanciation' behaviour should be equal and safe for end-users, despite that @MichaelSchneeberger you should run the test suite to check it's not harmful.

Also if we remove the singleton and technically allow sub-classing, we should clearly not support it in the issue tracker IMO, because it would be so easy to mess up with Local. Something like "CurrentThreadScheduler is not supposed to be sub-classed, do it at your own risk, you're on your own". That should be true for other schedulers by the way.

[dbrattli]

To resolve this I suggest that we implement things the same way as the Rx.NET CurrentThreadScheduler. They are using a static instance method to get the singleton behaviour, and also have a private contructor to make sure you cannot get multiple instances of the scheduler. Not that it would make a difference if you got 2 instances since the scheduler queue is static and shared among all instances. But the point is that you are never unsure about the semantics as with RxPY v1.6. For some it was a feature, for others it was a bug.

My suggestion if that we keep and enforce the current singleton behaviour for CurrentThreadScheduler and instead consider having a separate e.g TrampolineScheduler that supports the nested behaviour requested by @MichaelSchneeberger.

We should remove the current_thread_scheduler and instead expose it throgh an instance () class method or class property. Same goes for ImmediateScheduler. Private constructors are not possible with Python, so perhaps we should fail the scheduler if the user tries to instantiate it themselves. Not sure what it possible with Python.

[MainRo]

this should be fixed in #433