Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

EmbeddedEventLoop/Channel violates Sendable: Store the current thread in init() and then assert that we're on that thread #2949

Closed
weissi opened this issue Oct 24, 2024 · 5 comments · Fixed by #2951

Comments

@weissi
Copy link
Member

weissi commented Oct 24, 2024

EmbeddedChannel & EmbeddedEventLoop currently violate Sendable. They should store the current thread in init and then implement inEventLoop with precondition(self.myThread == Thread.current); return true.

Otherwise, EmbeddedChannel/EventLoop are just trivially violating the rules.

@Lukasa
Copy link
Contributor

Lukasa commented Oct 24, 2024

Yes, there's no dispute about this. They have always historically broken the rules, and that remains true today.

@weissi
Copy link
Member Author

weissi commented Oct 24, 2024

Yes, there's no dispute about this. They have always historically broken the rules, and that remains true today.

Right, I have no idea why we didn't put that into NIO 1.0... The real selectors (kqueue/epoll/uring) do actually have myThread and are asserting it -- despite the fact that they really don't need it, that would be an insane bug if that ever failed :P

@weissi
Copy link
Member Author

weissi commented Oct 24, 2024

@Lukasa Do you think this needs to be staged with a NIO_STRICT thing? This might break random test suites

@Lukasa
Copy link
Contributor

Lukasa commented Oct 24, 2024

Yeah, I think it does. Environment variable is an option, another option is to just have a static flag on EmbeddedEventLoop that you can use to flip the behaviour..

@weissi
Copy link
Member Author

weissi commented Oct 24, 2024

Yeah, I think it does. Environment variable is an option, another option is to just have a static flag on EmbeddedEventLoop that you can use to flip the behaviour..

cool, done

Lukasa pushed a commit that referenced this issue Oct 25, 2024
### Motivation:

`EmbeddedChannel` & `EmbeddedEventLoop` currently violate `Sendable`.
They should store the current thread in `init` and then implement
`inEventLoop` and other functions with a check that the current thread
is correct.
Since NIO 1.0 `Embedded*` were always documented to not be thread-safe
but we should finally police this.

### Modifications:

- Implement the thread check
- For now, just warn (soon hopefully crash)
- Delete EmbeddedScheduledCallbackTests (#2950)

### Result:

- Less Embedded* abuse
- fixes #2949
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants