impl Sync for SerialPort trait

[unlimitedsola]

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.

[sirhcel]

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.

[unlimitedsola]

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]

Is there anything that blocks this from getting merged?

[sirhcel]

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]

@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]

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

[unlimitedsola]

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?

Rebased, please take a look.

[sirhcel]

Thank you for rebasing @unlimitedsola! The changes proposed here are semver-breaking - at least in theory.

Therefor I would prefer checking the our visible dependents from crates.io and GitHub whether they would be affected by this change first. I did this already in the past and have to dust off the makeshift setup used there. But this will take some time.

[okhsunrog]

@sirhcel is there anything stopping it from being merged right now?

[sirhcel]

@sirhcel is there anything stopping it from being merged right now?

Yes, implementing Sync is actually a semver-breaking change. Cutting a release 5.0 just for this change seems a bit to much of semver compliance to me because I bet that this change does not affect depentents in practice. However, I want to give it a spin at least with our most-prominent dependents but I did not find the time for it the last weeks. ☹️

I don't know of tooling for this purpose (like a crater for libs) and used a modified get-all-crates the last time. But I killed all the scripting I created back then when deleting the server. I need to find some time to set it up again. Any hints to prior art are welcome @okhsunrog.