Skip to content

Move execution context event loop lock to each event loop#16520

Merged
straight-shoota merged 2 commits intocrystal-lang:masterfrom
ysbaddaden:feature/extract-evloop-lock-to-each-evloop
Dec 24, 2025
Merged

Move execution context event loop lock to each event loop#16520
straight-shoota merged 2 commits intocrystal-lang:masterfrom
ysbaddaden:feature/extract-evloop-lock-to-each-evloop

Conversation

@ysbaddaden
Copy link
Collaborator

This is a requirement for io_uring, and it improves readability despite a slight duplication.

Extracted from #16264

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Placing the lock on the event loop looks good.
But I'm wondering if we should move the implementation to EventLoop#lock? since it's shared across most subclasses. It's trivial to override with a no-op for io_uring.
That would put an unused @lock ivar in EventLoop::IOUring though 🤷
So maybe this is just fine.

@ysbaddaden
Copy link
Collaborator Author

ysbaddaden commented Dec 19, 2025

There could be a Crystal::EventLoop::Lock module that would implement the abstract methods a the general lock. That would avoid the duplication (and the override). Then io_uring would simply implement the methods.

@ysbaddaden ysbaddaden force-pushed the feature/extract-evloop-lock-to-each-evloop branch from 7c147ed to 6ee0c43 Compare December 22, 2025 14:32
@ysbaddaden
Copy link
Collaborator Author

Fixed conflicts with the master branch and added Lock module.

@straight-shoota straight-shoota added this to the 1.19.0 milestone Dec 22, 2025
@straight-shoota straight-shoota merged commit 7b2403d into crystal-lang:master Dec 24, 2025
41 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Multi-threading Dec 24, 2025
@ysbaddaden ysbaddaden deleted the feature/extract-evloop-lock-to-each-evloop branch December 24, 2025 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants