possible alternative to emit_que

[tlambert03]

@Czaki, here is a a possible alternative. it's quite similar to what you had, but allows for adding to the emit queue inside of a callback during emission.

one side observation, and perhaps this is a reason for your approach of disallowing reemission, is this test, where we make no special attempt to prevent a recursion error

def test_double_emmision_error():
    s = SignalInstance()

    def callback():
        s.emit()

    s.connect(callback)
    s.emit()

on main, that will fail eventually with a recursion error. On this PR, that somehow happily goes on forever, and python never raises a recursion error... it feels like we should still be able to catch that error, but i'm not sure how

[Czaki]

I will have time to test it tomorrow, but it does not look like it solve my example (I may be wrong)

[Czaki]

Signal.current_emitter is not thread safe, so depending on it in the emission mechanism may not be the best idea.

[tlambert03]

that's fine, it can be stored on the object itself and incorporated into the _lock

[Czaki]

Suggested change

	for caller in self._slots:
	try:
	caller.cb(args)
	except Exception as e:
	raise EmitLoopError(cb=caller, args=args, exc=e, signal=self) from e
	try:
	for caller in self._slots:
	caller.cb(args)
	except Exception as e:
	raise EmitLoopError(cb=caller, args=args, exc=e, signal=self) from e

for loop should be inside try-except to reduce usage cost.

[tlambert03]

that's on main... and sort of completely orthogonal to the discussion we're having here 🤪

[Czaki]

_run_emit_loop is used in 5 places in codebase. It is intentional to change only this place

[tlambert03]

just an example here for the sake of conversation... not a finished PR

[tlambert03]

but it does not look like it solve my example (I may be wrong)

if you mean the evented set example, then i'm pretty sure it did... Anyway, I'll wait till you've had time to actually have a closer look

[Czaki]

When doing fast review, I strongly assumed that this is implementing what you mentioned here: pyapp-kit#281 (comment), but I was wrong. You keep both queues.

one side observation, and perhaps this is a reason for your approach of disallowing reemission, is this test, where we make no special attempt to prevent a recursion error

No, I have added recursion exception for debug purpose. In situation when a user reports some problem with napari and some plugin, the option to enable such check will allow fasting detect event loop.
My code fails with recursion error, but such stacktrace is unreadable for me.

You use list.pop() which is slow in general. If we would like to use pop we should use Queue. I'm not sure if mypyc is happy with Queue, so I implement fast iteration using while loop.

Here is test that is passing on my PR and fail here:

def test_signal_order_suspend():
    """Test that signals are emitted in the order they were connected."""
    emitter = Emitter()
    mock1 = Mock()
    mock2 = Mock()

    def callback(x):
        if x < 10:
            emitter.one_int.emit(10)

    def callback2(x):
        if x == 10:
            emitter.one_int.emit(11)

    def callback3(x):
        if x == 10:
            with emitter.one_int.paused(reducer=lambda a, b: (a[0] + b[0],)):
                for i in range(12, 15):
                    emitter.one_int.emit(i)

    emitter.one_int.connect(mock1)
    emitter.one_int.connect(callback)
    emitter.one_int.connect(callback2)
    emitter.one_int.connect(callback3)
    emitter.one_int.connect(mock2)
    emitter.one_int.emit(1)

    mock1.assert_has_calls([call(1), call(10), call(11), call(39)])
    mock2.assert_has_calls([call(1), call(10), call(11), call(39)])

[tlambert03]

You use list.pop()

I used deque.popleft in a local version. Again, this wasn't meant to be a final performant version, it was merely about removing the exception you had added when emitting while emitting. That's it

[Czaki]

So excepetion could be just removed. It is additional change. I may open a separate issue/PR with debugging discussion

[tlambert03]

let's just focus on your PR. I'll close this. Can you make sure that all the tests we've discussed (such as test_signal_order_suspend and the EventedSet test) are in your PR over there? Let's just focus on tests and benchmarks, and worry about the implementation details later