Skip to content

Improve generator for concurrent access - #2151

Merged
BCSharp merged 2 commits into
IronLanguages:mainfrom
BCSharp:generator-busy
Nov 7, 2026
Merged

BCSharp merged 2 commits into
IronLanguages:mainfrom
BCSharp:generator-busy

Conversation

@BCSharp

@BCSharp BCSharp commented Nov 6, 2026

Copy link
Copy Markdown
Member

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.

@slozier slozier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. Don't think I want a "long" hammer test, but an event driven one might work?

@BCSharp

BCSharp commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

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 ipy process directly, but skips if run by the test harness. I thing there are more tests that are currently disabled because they are too long, but are still useful when someone runs the test file directly.

@slozier

slozier commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

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])

@BCSharp

BCSharp commented Nov 7, 2026

Copy link
Copy Markdown
Member Author

Claude proposed this to me

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.

@slozier

slozier commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Well, Claude got really good at writing test code, didn't it?

Yes, they all got very good at a lot of things. It's a bit disheartening, but at the same time very cool.

@BCSharp
BCSharp merged commit 8c1e2f6 into IronLanguages:main Nov 7, 2026
35 checks passed
@BCSharp
BCSharp deleted the generator-busy branch November 7, 2026 04:41
@BCSharp

BCSharp commented Nov 7, 2026

Copy link
Copy Markdown
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants