Skip to content

looper: Require Send when adding fd event callbacks on ForeignLooper#469

Merged
MarijnS95 merged 1 commit intomasterfrom
looper-closure-send
Apr 26, 2024
Merged

looper: Require Send when adding fd event callbacks on ForeignLooper#469
MarijnS95 merged 1 commit intomasterfrom
looper-closure-send

Conversation

@MarijnS95
Copy link
Member

ForeignLooper allows (re)registering callbacks on threads that are not the ThreadLooper (current) thread. Since these calls will be executed on that ThreadLooper thread when the user calls any of the poll() functions, these closures have to pass a thread boundary which requires Send.

@MarijnS95 MarijnS95 requested a review from rib January 23, 2024 08:37
@MarijnS95 MarijnS95 added the impact: breaking API/ABI-breaking change label Jan 23, 2024
@MarijnS95 MarijnS95 force-pushed the looper-closure-send branch from 09fc908 to ecf32a1 Compare January 26, 2024 09:40
Copy link
Member

@rib rib left a comment

Choose a reason for hiding this comment

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

Nice, this looks good to me!

Comment on lines +242 to +243
// let callback = unsafe { std::mem::transmute(callback) };
// self.foreign.add_fd_with_callback(fd, events, callback)
Copy link
Member Author

Choose a reason for hiding this comment

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

A transmute doesn't seem to work here to deduplicate the two functions, unfortunately :/

@MarijnS95
Copy link
Member Author

MarijnS95 commented Feb 4, 2024

Found a way, but I think add_fd_with_callback_assume_send() should be unsafe. done.

`ForeignLooper` allows (re)registering callbacks on threads that are not
the `ThreadLooper` (current) thread.  Since these calls will be executed
on that `ThreadLooper` thread when the user calls any of the `poll()`
functions, these closures have to pass a thread boundary which requires
`Send`.
@MarijnS95 MarijnS95 force-pushed the looper-closure-send branch from 6d35e23 to 9aa58be Compare February 4, 2024 16:45
@MarijnS95 MarijnS95 requested a review from rib February 4, 2024 16:45
@MarijnS95 MarijnS95 merged commit 6893a70 into master Apr 26, 2024
@MarijnS95 MarijnS95 deleted the looper-closure-send branch April 26, 2024 12:16
- native_window: Require linking against `libnativewindow` for most API >= 26 functions. (#465)
- **Breaking:** audio: Merge `AudioResult` variant enum into `AudioError`. (#467)
- data_space: Add missing `DataSpaceRange::Unspecified` variant. (#468)
- **Breaking:** looper: Require `Send` marker when adding fd event callbacks on `ForeignLooper`. (#469)
Copy link
Member Author

Choose a reason for hiding this comment

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

As soon as I click merge, I realized I should have probably mentioned that ThreadLooper::add_fd_with_callback() was added without the Send bound, if already on the same thread.

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

Labels

impact: breaking API/ABI-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants