Fix shared locks #314

[VMshall]

Fixes #314.

Currently, the library unconditionally requests an exclusive lock when opening a serial port (flock::lock_exclusive on POSIX and share_mode = 0 on Windows). This prevents scenarios where two shared TTYPorts or COMPorts need to be opened simultaneously (e.g., when working with pseudo-terminals).

• Added an exclusive: bool field to SerialPortBuilder, defaulting to true to maintain backwards compatibility.
• Added a pub fn exclusive(mut self, exclusive: bool) -> Self builder method.
• POSIX (src/posix/tty.rs): Updated TTYPort::open to check builder.exclusive. If true, it applies ioctl::tiocexcl and flock::lock_exclusive. If false, it applies ioctl::tiocnxcl and flock::lock_shared.
• Windows (src/windows/com.rs): Updated COMPort::open to check builder.exclusive. If true, it uses a share_mode of 0. If false, it uses FILE_SHARE_READ | FILE_SHARE_WRITE.

This allows users to explicitly opt-out of exclusive locking when they need shared access to a port.

[sirhcel]

Thank you for this PR @VMshall! Looking at you second commit c0aa072: Might this just be an accidental check-in of some files from you working directory? In case it is, you can reset you branch to f1759e5 and update the PR by force-pushing to the branch.

Please ignore build errors from our MSRV targets. The are caused by our pretty old MSRV and updated dependencies which do not consider MSRV changes semver-breaking.

[sirhcel]

Is committing the files from c0aa072 really intended?

[RossSmyth]

I'm unsure of the value of this API, I don't particularly see the point? Could you explain your motivation a bit more? From a win32 perspective I don't really see the point in carrying more data in the struct, if you open it exclusively you do so explicitly, not too much point in carrying that data along for the entire lifetime of the structure.

It would probably be more worthwhile to add an API to add os-specific opening flags like in std.

[RossSmyth]

While this is legacy, this is not conservative. It should really default to false.

Plus it is possible to determine.

[VMshall]

Thanks for the review, Ross!

The motivation behind the exclusive option in the builder was to provide a cross-platform way to opt out of exclusive locking, as unconditionally acquiring an exclusive lock (flock) on Unix was preventing users from opening two shared TTYPorts. I agree there's no need to carry the state around for the lifetime of the struct. I will update the PR to remove the exclusive field and getter from both COMPort and TTYPort and only use it at open-time in the builder.

[sirhcel]

While this is legacy, this is not conservative. It should really default to false.

Is it actually? Doesn't COMPort open the device file in exclusive mode as of now?

[RossSmyth]

Just because our default is exclusive doesn't mean it is conservative. You can create a COMPort from an external handle via FFI that we do not control (and I do in several projects), and at that time you have no idea how it was opened.

[RossSmyth]

I imagine there is a similar API on Linux, though I do not use this library for Linux applications much.

[sirhcel]

Just because our default is exclusive doesn't mean it is conservative. You can create a COMPort from an external handle via FFI that we do not control (and I do in several projects), and at that time you have no idea how it was opened.

Good point. And to me this makes another case for crafting eventual support for checking exclusivity more carefully. At a first glance, this seems not be trivially possible on Window. However, we could navigate around this by returning an Option<bool> with an actual value for the case the file has been opened by us and None for port instances created from raw handles/file descriptors.

[sirhcel]

Thank you for continuing the discussion in the meantime @RossSmyth and @VMshall!

It would probably be more worthwhile to add an API to add os-specific opening flags like in std.

Could you give us a reference to that? Do you mean OS-specific flags on the builder?

I'm unsure of the value of this API, I don't particularly see the point? Could you explain your motivation a bit more? From a win32 perspective I don't really see the point in carrying more data in the struct, if you open it exclusively you do so explicitly, not too much point in carrying that data along for the entire lifetime of the structure.

I see value an adding support for allowing the user to configure file locking for a serial port. It's there, always on and I know the following two reports where this stands in the way of users who want to read and write from separate threads: #309 and #314.

And of course, the demand stems from the lack of support for splitting a serial port instance into something like a read and write half for this use case. But even if the cases above are not the "right" motivation for making locking configurable, what would be your case against adding the option to the builder @RossSmyth?

I'm just referring to tho builder so far. From my point, it's perfectly fine to postpone the decision on how to add this to the platform implementations. I see the point in adding support for querying the locking status @VMshall. It's there for the Posix backend so why not for Windows too? The runtime cost for carrying the flag around in the TTYPort struct as well seems neglectable to me. Do you see actual issues with that @RossSmyth?

What bothers me more is that there is the presence of TTYPort::set_exclusive for changing exclusivity during the lifetime of a port. And the complete absence of this concept in the trait.

I would opt for adding support for configuring locking from the builder and discuss and develop the rest in an separate issue (in the context of #302). With that said, do you have a point with making locking configurable through the builder and postpone API changes to the port implementations and the serialport trait @RossSmyth?

[RossSmyth]

Maybe I just don't use my serial port the same way, but I do not really see any way to have the same serial port open more than once in a way that doesn't result in junk data. I guess the one issue linked it is just writing on one thread and reading on the other, which just gets buffered in the kernel so nothing can really be raced. But without enforcing that reading and writing at the API level it seems fragile to me.

The runtime cost doesn't matter too much since I imagine no one opens thousands of serial ports, so doesn't really matter too much.

As for os-specific APIs, I was referring to these
https://doc.rust-lang.org/std/fs/struct.File.html#impl-FileExt-for-File

[VMshall]

Thanks for the review and discussion, @RossSmyth and @sirhcel i completely agree with the points raised there's no need to carry the exclusive state throughout the lifetime of the COMPort or TTYPort structs, especially since we can't reliably determine the state from raw handles anyway. I've just pushed a new commit that implements your feedback ,removed the exclusive field entirely from both COMPort and TTYPort. removed the exclusive() getter from both platforms, and removed the set_exclusive() method from TTYPort. Builder only configuration: Exclusivity is now strictly an opentime configuration handled solely by SerialPortBuilder. It evaluates the flag at the time of opening and discards it.

[sirhcel]

Thank you @VMshall, I'm keen on having a look at your update but I can't find it here in the PR nor in your fork. Where did you push the changes to?

I've just pushed a new commit that implements your feedback ,removed the exclusive field entirely from both COMPort and TTYPort. removed the exclusive() getter from both platforms, and removed the set_exclusive() method from TTYPort.

Sorry to interrupt here: Please leave the existing exclusivity flag and TTYPort::set_exclusive in place. Let's just add the configuration option to the builder and leave the rest as-is. Otherwise we would introduce as semver-breaking change which I want to leave for the refactoring from #302 and get this fix into an 4.x release.

[VMshall]

Thanks for the clarification! I'll restore the exclusivity flag and TTYPort::set_exclusive, and update the PR to add the configuration option to the builder without removing the existing API.

[VMshall]

Thanks for the clarification, @sirhcel

I've updated the PR to adhere to semantic versioning for the 4.x release line. The existing TTYPort exclusivity field and set_exclusive() method have been restored and left unchanged. The exclusive field and getter I had added to COMPort in this PR have been removed, so it now pulls the value from the builder at open time. The builder-based configuration for exclusivity is retained.

Please let me know if anything looks off.

[sirhcel]

Thank you for the cleanup!

Please let me know if anything looks off.

Yes, I'm wondering what's behind merging the main branch back into the PR branch? My gut instinct would have it kept it a sideline to main for rebasing.

Could you please add an entry to the changelog?

[VMshall]

I merged the main branch to synchronize the PR and ensure our recent fixes pass all checks against the latest upstream changes. It was done primarily via git pull which defaults to a merge commit. I understand you'd prefer a cleaner commit history! I'd be happy to rebase the branch onto main and force-push if you'd like, or I can just drop the merge commit. Let me know what you prefer!

[sirhcel]

Thank you for the changelog @VMshall! I refactored the commits to what I had in mind with 0fb7a3b4:

• The core change (without introducing unrelated noise from adding a comment to a place not touched by it)
• The changelog entry
• A small cleanup to the formatting (which slipped through likely due to too much noise from expectedly failing CI checks)
• And a test for the new configuration option SerialPortBuilder::exclusive

My update should have gone to a separate brach but ended up directly in this PR. I'm about to refactor the tests for locking to cover the exclusivity setting and will add them soon.

Obsoleted by the updates to this PR.

[sirhcel]

Here they finally are, the tests. And it has been an interesting journey: I learned that on Windows, using a shared lock for COM ports is not supported on Windows. This explains why there was no support for configuring a locking mode on Windows available so far.

This means that we need to adapt our configuration approach where SerialPortBuilde::exclusive is currently generally available. At a first glance, eihter by making this method only conditionally available when building for POSIX targets or by means of an extension trait. I've already seen the latter in the standard library. Let's do some overview research whether the former is also common.

Do you want to give the aforementioned refactoring a shot @VMshall?

[VMshall]

@sirhcel Yes, I'd love to give it a shot!