Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: provide send-wrapper to contidionally implement Send for operators #4443

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

waynexia
Copy link
Member

@waynexia waynexia commented Apr 9, 2024

Closes #4437

Quote from #4437:

wasm32-unknown-unknown only has one worker thread, it's wise to use !Sync structure under that precondition. But since this marker trait would affect others, it's sometimes hard to use them in a "normal" codebase (i.e., write for multi-thread but run in wasm32-unknown-unknown).

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@Xuanwo
Copy link
Member

Xuanwo commented Apr 9, 2024

Thank you for the PR. I'm wondering if we have more maintainable implementations.

For instance, let's define a trait named IntoSend with a method into_send() and implement it for futures. We could then wrap the input using send_wrapper, effectively making it a no-op on other targets.

What do you think?

@waynexia
Copy link
Member Author

Sounds great, let me have a try 👍

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia
Copy link
Member Author

Please take another look @Xuanwo

I use cargo check --target wasm32-unknown-unknown --features send_wrapper to verify if it works. Do you think we need to add this in CI?

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo
Copy link
Member

Xuanwo commented Apr 22, 2024

I use cargo check --target wasm32-unknown-unknown --features send_wrapper to verify if it works. Do you think we need to add this in CI?

Feel free to do this in a new PR!

@Xuanwo Xuanwo merged commit 525c512 into apache:main Apr 22, 2024
25 checks passed
@waynexia waynexia deleted the send-wrapper branch April 22, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditionally implement Send and Sync for wasm32-unknown-unknown target
2 participants