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

impl Sync for SerialPort trait #233

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

unlimitedsola
Copy link

@unlimitedsola unlimitedsola commented Dec 12, 2024

Closes #232

There are many alternatives that don't involve unsafe impl, but I believe this is the simplest one that doesn't involve an MSRV bump, nor should it introduce breakage, at least I hope...

One alternative is to use OwnedHandle provided by stdlib. However, doing so requires an MSRV bump to at least 1.63.0, which I've heard would require a major version bump in this library so I refrained from this approach.

Copy link
Contributor

@sirhcel sirhcel left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR! Please see my comment below. I'm wondering what's you use case for sharing &SerialPort between threads? At a firs glance there are only a few meaningful things to do with an immutable reference.

src/windows/com.rs Outdated Show resolved Hide resolved
@unlimitedsola
Copy link
Author

I'm wondering what's you use case for sharing &SerialPort between threads? At a firs glance there are only a few meaningful things to do with an immutable reference.

I was trying to display some port information and settings that should only require an immutable reference in another thread. Without impl Sync, however, it will force me to pass ownership to that thread and then get it back.

@okhsunrog
Copy link

Is there anything that blocks this from getting merged?

@sirhcel
Copy link
Contributor

sirhcel commented Jan 22, 2025

Is there anything that blocks this from getting merged?

Yes, @okhsunrog, starting implementing Sync is a (potential) semver-breaking change and we have to somewhat carefully look into it. Just for curiosity, what's your use-case?

I already added semver checks to CI with #240 but I'm apparently not entitled to rebase this PR on the current upstream master. @unlimitedsola, could you please rebase this PR on upstream/master or allow edits by maintainers?

I've started such checks a while ago but as of now, they require a decent amount of manual care. Will try to set them up and run them for this PR soon(TM).

@okhsunrog
Copy link

@sirhcel I used this crate via tokio-serial crate, and it worked fine on Linux. Once I tried to compile my app for Windows, I saw the error that SeriaPort is not Sync on windows. So, for now I have this in my code:

    // SerialStream from tokio-serial crate is not Sync on Windows, so we need to spawn a separate thread for mmw_lib
    #[cfg(windows)]
    {
        // Create a new single-threaded runtime for Windows
        let runtime = tokio::runtime::Builder::new_current_thread()
            .enable_all()
            .build()
            .unwrap();

        // Spawn the radar connection handler on the new runtime
        std::thread::spawn(move || {
            runtime.block_on(async {
                radar_connection_handler(rx_radar_control).await;
            });
        });
    }

    #[cfg(not(windows))]
    {
        // Spawn a new tokio task to manage radar connection (original behavior for non-Windows platforms)
        tokio::spawn(async move {
            radar_connection_handler(rx_radar_control).await;
        });
    }

@okhsunrog
Copy link

@sirhcel so the answer is: it can be only used in a single-threaded tokio runtime on Windows, sadly

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.

Consider impl Sync for SerialPort trait
3 participants