pyapp-kit / psygnal

Python observer pattern (callback/event system). Modeled after Qt Signals & Slots (but independent of Qt)
https://psygnal.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
84 stars 13 forks source link

fix: ensure proper order of signal emission #281

Closed Czaki closed 8 months ago

Czaki commented 8 months ago

I have noticed that If some callback triggers emission of an event during an existing loop, the events may come in incorrect order, leading to a corrupted state.

I think that it is a bug, but I'm not sure if my approach to solve is correct.

codspeed-hq[bot] commented 8 months ago

CodSpeed Performance Report

Merging #281 will degrade performances by 10.47%

Comparing Czaki:proper_signal_order (eccef1a) with main (4aa62b0)

Summary

❌ 1 (👁 1) regressions ✅ 65 untouched benchmarks

Benchmarks breakdown

Benchmark main Czaki:proper_signal_order Change
👁 test_emit_time[setattr-50] 200.7 µs 224.1 µs -10.47%
codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (4aa62b0) to head (eccef1a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #281 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 22 22 Lines 1880 1883 +3 ========================================= + Hits 1880 1883 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tlambert03 commented 8 months ago

minor modification suggested in https://github.com/Czaki/psygnal/pull/1 it's not perfect (see note about recursion error), but curious if you have any thoughts on that approach vs this current one

tlambert03 commented 8 months ago

please merge main here and make sure all the tests we've discussed are added, then ping for review

Czaki commented 8 months ago

@tlambert03 compiled test are ending with Segfault. Did you meet something like this before:

  File "/Users/runner/work/psygnal/psygnal/tests/test_psygnal.py", line 990 in test_recursion_error
tlambert03 commented 8 months ago

no, I haven't seen that before. I can reproduce locally as well though (was just trying as you wrote), but not on main. (btw, to test locally, you can run make build, and make clean to undo ...)

Czaki commented 8 months ago

Ok. The problem is that hatch/mypyc performs compilation in tmp directory (unlike cython that places c/cpp files in the structure of packages. So the debugger cannot find the source files.

I will try to dig, but maybe you know how to change this behavior or disable cleaning tmp

#7  0x00007ffff508f242 in CPyDef__signal___SignalInstance____run_emit_loop ()
    at /tmp/tmpcocqfl4i/build/__native_ac51d50a4f4b6d748b8c.c:37311
37311   /tmp/tmpcocqfl4i/build/__native_ac51d50a4f4b6d748b8c.c: No such file or directory
tlambert03 commented 8 months ago

good sleuthing. i also don't know, but happy to look into it if you can't find it easily

Czaki commented 8 months ago

Im going to Warsaw Python meeting so will not be active today. Will sit to this at evening or toomorow