Repository navigation
Improve generator for concurrent access - #2151
Conversation
slozier
left a comment
There was a problem hiding this comment.
Seems fine to me. Don't think I want a "long" hammer test, but an event driven one might work?
I don't think an event-driven will work without at least some hammering. What we can do is a decorator that lets the test run if called by the |
|
Claude proposed this to me, and it does fail before and pass after. So if you think it addresses the issue you're fixing feel free to use it. Maybe your hammer covers another aspect of things in which case it might be useful? Concurrency is giving me a headache. 🥲 def test_entry_while_running_on_other_thread(self):
entered, release = threading.Event(), threading.Event()
def g():
entered.set()
release.wait()
yield 1
yield 2
gen = g()
results = []
t = threading.Thread(target=lambda: results.append(next(gen)))
t.start()
self.assertTrue(entered.wait(10))
self.assertEqual(gen.gi_running, 1)
for f in (lambda: next(gen), lambda: gen.send(None), lambda: gen.throw(KeyError), gen.close):
self.assertRaises(ValueError, f)
release.set()
t.join()
self.assertEqual(gen.gi_running, 0)
self.assertEqual(results, [1])
self.assertEqual(list(gen), [2]) |
Well, Claude got really good at writing test code, didn't it? Your test does test a number of improvements in this PR, some of them I haven't even tested for. Though it does not test for the race in the check the hammer test was testing. But the correctness of that could also be concluded by simply analyzing the code. All in all, I like your test and will use it in the next commit. |
Yes, they all got very good at a lot of things. It's a bit disheartening, but at the same time very cool. |
I share your sentiment. I am actually not surprised that it got good at writing linear code (like tests) but that it starts to grasp concurrency (my favourite, together with async) is impressive. I wonder how far we can push the limits. |
This change makes the generator check-and-set an atomic operation, so that only one thread at a time can claim the generator and modify its fields.
The code flow in this file is not necessarily what I would choose, but I stayed with what is already there to make the diff simpler.
Since this is a concurrency fix, a test requires a generator being hammered by two threads for at least several seconds, to trigger a failure (in the old code), and even then it can produce false negatives (the longer, the less likely though). @slozier, let me know it you think a hammer test is needed (count ~5s extra for it); I have it locally.