Provide a try_clone() method for ports/devices

[apoloval]

Some IO types of standard library as std::net::TcpStream support cloning. This is necessary to implement some non-trivial communication patterns in which one thread performs the reads and a different one performs the writes.

serial-rs lacks this functionality. It would be great to have a try_clone(&self) function in SerialPort and SerialDevice that makes possible to clone the port so copies can be used by different threads.

[dcuddeback]

@apoloval That's a valid use case, and actually it's one that I'm going to run into on an upcoming project. I'm not convinced that try_clone() is the best solution for this crate, though. As much as possible, I agree with trying to be consistent with the standard library, but adding try_clone() to the SerialDevice trait assumes that all implementations have an underlying file descriptor or handle running on an OS that supports a clone operation. That may be true for TTYPort and COMPort, but serial ports are defined by a trait so that new implementations can be added. I'm uneasy placing the requirement of try_clone() on new implementations, as some implementations may not even have an OS.

[apoloval]

I agree cloning is not available in all implementations and that should be considered. Standard library authors agree as well with TcpStream. That's why the signature of the function in std::net is:

fn try_clone(&self) -> Result<Self>

In case the underlying implementation is not able to duplicate the serial port, this operation would fail with the appropriate error. New implementations can be added that does not support cloning.

My proposal is to provide the same signature to the try_clone() method of SerialDevice and SerialPort, as the submitted PR does.

Do you still think this is not enough to dispel your concerns about future implementations?

[vvanders]

I'm looking at a similar use case where I want to block + read from one thread why sending from another.

Rather than adding a new API why not let the respective serial implementations mark their struct as Sync+Send?

AFAIK both win32 ReadFile/WriteFile are threadsafe, as are read/write. This would let you do the read/write from separate threads and allow platforms future platforms that don't allow this to mark their implementation appropriately.

Happy to work on a PR+testing if you think this is a viable path.

[dcuddeback]

@vvanders @apoloval I've been working on a crate that I thought would motivate this use case, but I ended up needing to wrap I/O in a mutex anyway.

Implementing Sync is an interesting idea. Unfortunately, the Read and Write stdlib traits take &mut self. I have thought about moving I/O methods into the serial port traits to support #![no_std]. That would make Sync markers a possible solution. I just did a quick test. It looks like Sync is already implemented for TTYPort, but not yet for COMPort. @vvanders Do you have any idea if EscapeCommFunction(), GetCommModemStatus(), GetCommState(), and SetCommState() are thread safe?

Another idea could be to provide try_clone() in a separate trait (e.g., TryClone). That'd be similar to Sync in a way, because the caller would specify whether they require the extra bound: fn foo<S: SerialPort + TryClone>(port: S) or fn foo<S: SerialPort + Sync>(port: S).

[vvanders]

Hmm, no clue on EscapeCommFunction() and related. I would assume so given that most HANDLE functions are but the docs are incredibly sparse WRT thread safety.

I was thinking about this a bit more and if we really wanted to be correct it seems like you only want one reader + one writer. Otherwise the semantics around reading/writing are a little weird. However that ends up complicating/changing the API significantly and I'm not sure if you want to go through the deprecation/work.

I kinda like the try_clone() as a trait that you mentioned. Seems flexible and matches what the underlying APIs somewhat expect. I really like that callers can use it to constrain their signatures. It's also less invasive than a whole new read/write API.

[dcuddeback]

@vvanders Yeah, I couldn't find much in the docs regarding thread safety, and I'm not very familiar with Windows APIs in general. It sounds like it may be possible to mark COMPort as Sync, but I'd like to know more before I'd be comfortable doing that. It would also require making methods take &self instead of &mut self.

I thought about defining separate readers and writers, too, maybe with a method like fn split(self) -> (Reader, Writer), but it's not clear which object would own the control lines (DTR, RTS, etc). I can't think of a use case where the control lines would be used in a multithreaded application. There's also a complication of the Drop trait which closes the port. Either the port is closed when the last of Reader and Writer go out of scope (requiring Arc), or Reader and Writer must borrow SerialPort, which could complicate multithreading.

I'm leaning toward adding TryClone as a trait.

[apoloval]

For my specific use case, having a TryClone trait would be great. Please remember this is something I implemented in my fork, which is the one I'm using right now in my use case. Perhaps you might find it useful.

[dcuddeback]

So I got something working for Unix TTY ports: https://github.com/dcuddeback/serial-rs/compare/feature/try-clone. It's not as simple as I expected because of the TIOCNXCL ioctl in the Drop impl. I had to add reference-counting to ensure that exclusive mode is only reset when the last instance is dropped.

It's also not clear from reading the man pages whether this scenario is supported:

{
  let port2 = {
    let port1 = serial::open("/dev/ttyUSB0").unwrap(); // ioctl(port1, TIOCEXCL);
    port1.try_clone().unwrap() // port2 = dup(port1);
  }; // close(port1);
} // ioctl(port2, TIOCNXCL); close(port2);

The part that concerns me is that TIOCNXCL is called on a different file descriptor than TIOCEXCL. It seems to work okay when I tested it, but will need to verify on other Unixes.

@apoloval With the version in your fork, I was able to open the same port from two programs:

let port1 = serial::open("/dev/ttyUSB0").unwrap();

{
  let port2 = port1.try_clone().unwrap();
}
// port2 is closed here, which executes the TIOCNXCL ioctl,
// releasing the port to other programs while port1 is still open

[gbip]

So is this still on it's way ? It would be really neat to have the ability to have one thread reading from a serial connection, and the other one writing to it !

[dcuddeback]

@gbip For your use case, you can probably work around it by wrapping the port in Arc<Mutex<T>>. That's what I've done whenever I had this use case, because I had to synchronize reads and writes anyway in order to associate reads with the corresponding write.

That said, there are some use cases that require cloning (e.g., using BufReader as mentioned in #40), so this is still on the table. No promises when it will land though.

[gbip]

That is what I have done, but then to write stuff I have to wait until my reading function returns, which can take quite some time.

Is there a way I could help with this feature ? I am not very familiar with file descriptors and things like that, but I would be glad to at least try helping you !

[inventumamet]

I think since Arc<Mutex> lock is scope lock, you could try this in your thread:

let mut port = serial::open("COM6")
                    .expect("Error opening serial port");

// do your configuration and settings for the port here

// then "move" the port to a mutex:
let port_handle = Arc::new(Mutex::new(port));

let my_read_thread = thread::spawn(move || {
    let mut buffer = [0u8; 32];

    loop { // loop forever
        let mut read_result:  Result<usize, Error>;
        { // this scope will reduce time on read
            read_result = port_handle.lock().unwrap().read(&mut buffer);
        }
        // the scope is gone and mutex is unlocked
        match read_result {
            // now we deal with the result
        }
    }
});

Then similarly for the write thread. But you will need to clone() the mutex for the write thread.

This way the read and write is not truly independent, but at least you get to minimise the time spent trying to do it.

I still prefer using a clone if possible. Or maybe we need to update the documentation? I am happy to share the example code I made. https://gist.github.com/lowlifer/3166de711680006da42661256f926e23

I could help with Windows Duplicate Handle stuff and testing. Do you mind if I play around a bit forking the feature branch you got?

[gbip]

This feature was implemented recently on serialport-rs if you need some inspiration !

[dcuddeback]

I could help with Windows Duplicate Handle stuff and testing. Do you mind if I play around a bit forking the feature branch you got?

@Lowlifer That would be great! I could use help with Windows. It may even be easier than Unix, since Windows automatically locks COM ports when they're open and doesn't require a userland call to ioctl(). I'm just much less familiar with the Windows API than I am with Unix. As I see it, the two issues that need to be verified/tested are:

1. Is having multiple handles to the same device thread-safe? I assume this should be safe, since Windows provides a DuplicateHandle function.
2. Does the COM port remain locked until the last handle is closed, regardless of the order in which they're closed?

I believe the feature branch you're referring to only implements try_clone() for TTYPort, but @apoloval has a branch that implements it for COMPort, too: https://github.com/dcuddeback/serial-rs/pull/22/files#diff-eb653cfea4641b0accb1fdfa7a56f6f3R141.

[dcuddeback]

@gbip I'd appreciate it if you don't plug other projects here. I'm aware of serialport-rs. It's actually a fork of this project. Besides violating this project's MIT license by deleting the license text (https://github.com/dcuddeback/serial-rs/blob/master/LICENSE#L11-L12), they've also rewritten their git history for some reason, and then they relentlessly spam my projects to promote theirs. That's not the kind of project I would normally take inspiration from.

If there's something novel in that project that addresses the issues discussed in this ticket, I'd appreciate it if you could enlighten us on the solution. However, it looks to me like they're doing the same dup()/DuplicateHandle() approach that we've already seen in this ticket, and that has been shown to conflict with TIOCEXCL without any extra resource management. I'm not sure what inspiration we're supposed to take from that.

[inventumamet]

1. Is having multiple handles to the same device thread-safe? I assume this should be safe, since Windows provides a DuplicateHandle function.
   

I'm just trying to come up with a reliable way of testing this. I was thinking maybe I could clone the handle twice and then try writing to two handles from two difference threads, and then ensure the reads show all the characters I have written. So writer thread 1 writes number 1-9 and writer thread 2 writes letters a to i. The reader should get all the unique characters, 18 in total.

What are you thoughts on my scenario? Please suggest any improvements =)

2. Does the COM port remain locked until the last handle is closed, regardless of the order in which they're closed?
   

This should be relatively easy to test. I can have a separate process which will try to connect to the same serial port after using serial-rs to opened the port. It should continue failing to connect until I have closed all the clones from serial-rs.

I will get started on test 2 first. And then do test 1 as I have described.

[inventumamet]

@dcuddeback okay, dumb question, and off topic, but how do I get the diff from the link you gave me to apply to the git fork I just made?

dumb question 2: how do i build this on windows?

[dcuddeback]

@Lowlifer

but how do I get the diff from the link you gave me to apply to the git fork I just made?

You can add .diff or .patch to the end of a PR's URL and then apply the patch with git apply. Unfortunately, that PR was for an old version of this library and I've extensively reorganized the code base since then. The concept isn't specific to the latest HEAD, so you can checkout an older commit from around the time of that PR to apply the commit. 28ea4cc04dd27d67472dac96d86a0f8f1ac0c945 seems to work for me.

$ git checkout 28ea4cc04dd27d67472dac96d86a0f8f1ac0c945
$ wget https://github.com/dcuddeback/serial-rs/pull/22.patch
$ git apply 22.patch

That should be good enough for testing. I wouldn't put the work into porting a solution to HEAD until we know that it's going to work.

dumb question 2: how do i build this on windows?

Should be just cargo build. It's only dependency is the libc crate.

This should be relatively easy to test. I can have a separate process which will try to connect to the same serial port after using serial-rs to opened the port. It should continue failing to connect until I have closed all the clones from serial-rs.

Yeah, this is the easier one to start with. I would test closing the COM ports out of order as well as in order.

What are you thoughts on my scenario? Please suggest any improvements =)

That seems like a decent test. My only suggestion would be that it would likely take more than 18 characters to notice any thread-safety issues. Maybe pipe a couple of large files through each thread.

There's also the functions EscapeCommFunction(), GetCommModemStatus(), GetCommState(), and SetCommState() which are used in reading/writing the port settings and control lines. It's less obvious how to test those.

For both questions, it'd be great to have an answer from the documentation. Since these questions could be dependent on the hardware drivers, it'd be great to know if Microsoft has said if it should be safe to call these from different threads. That's where it would be helpful to me if someone more familiar with the Windows API could chime in.

[inventumamet]

@dcuddeback

All good and built as per your instructions. Thank you so much. Your instructions were really clear and it was really easy to follow from there.

Does the COM port remain locked until the last handle is closed, regardless of the order in which they're closed?

I performed this test and yes it passes. Opened one handle and try_clone() another and regardless of order of closing the port is locked until both handles have drop()ped. I am currently using a different programming language to try to open the port while it was locked. I will re-write the test to use serial-rs only. And need to add some communications to see the port is still working while locked.

My assumptions is that ideally you would like to add this test to the code when I'm done?

Is having multiple handles to the same device thread-safe? I assume this should be safe, since Windows provides a DuplicateHandle function.

Let me read the MSDN documentation and get back to you.

[inventumamet]

@dcuddeback
So after reviewing the MSDN documentation I'm pretty confident that it is thread safe, even though it is not explicitly mentioned. This is because of the example it gave.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251(v=vs.85).aspx

The example was on duplicating a mutex handle. So the mutex object was created and duplicated to be passed into a child thread. The main thread and child thread now shares the same handle to the same mutex. I'm guessing that this example wouldn't be given if it weren't thread safe.

[inventumamet]

My plan for testing thread safety for writing to the handle

// The communication will be between COM5 and COM6
//
// This code will first open COM5 and clone it so there are two COM5 handles
// Then it will open a single COM6 handle
//
// It will spawn three threads, one for each handle.
// In two of the threads with COM5 handles it will write to the handles
// first thread will write 0xAA - 10 million times
// second thread will write 0x55 - 10 million times
//
// In the thread with COM6 handle the data will be read into a buffer
// The buffer will be passed to the main thread when finished
// The main thread will parse through the buffer and be expecting the following:
// 20 million bytes in total
// 10 million bytes are 0xAA
// 10 million bytes are 0x55
//
// the aim is to see if there are data corruption, any corruption will point
// to the possibility of a race condition

[inventumamet]

@dcuddeback Okay so the above test passed.

Now I have these two tests, what's the best way to keep it for future use so someone else doesn't have to repeat what I did?

Also, I needed to impl Sync for COMPort in order for the cloned handle to be passed into threads.

[dcuddeback]

@Lowlifer Thanks for taking the time to research this. This is helpful.

I performed this test and yes it passes. Opened one handle and try_clone() another and regardless of order of closing the port is locked until both handles have drop()ped.

Good to know. This will probably make the Windows implementation a little simpler than the Unix one. In case this is dependent on hardware drivers, would you mind sharing what serial port hardware you tested with? For example, is it a serial port built into your motherboard, an addon card, or (more likely these days) some kind of USB to serial converter? FTDI? Prolific? Another vendor? (Hopefully it's something different than the hardware I have available to test with.)

I am currently using a different programming language to try to open the port while it was locked. I will re-write the test to use serial-rs only.

It's actually better that you tested with a different programming language, because the point of locking the port is to be inter-operable with the rest of the platform.

The example was on duplicating a mutex handle. So the mutex object was created and duplicated to be passed into a child thread. The main thread and child thread now shares the same handle to the same mutex. I'm guessing that this example wouldn't be given if it weren't thread safe.

Hmm... the whole point of a mutex is to implement thread-safety, so that doesn't really say anything about COM ports.

Okay so the above test passed.

That's a pretty good indicator as well.

Now I have these two tests, what's the best way to keep it for future use so someone else doesn't have to repeat what I did?

Are you referring to the test code you wrote? You could paste it as a Gist and link to it from this thread. Or if it's short, you could just paste it inline in a comment in this thread, too.

[inventumamet]

Hi @dcuddeback

Sorry for the late reply. Got busy lately.

https://gist.github.com/lowlifer/7c7628106308356f7766ce2fc58a44e2

Please see the gist with my test code.

[Pavlos1]

@dcuddeback A simple solution to something like #40 is to let the user choose whether they want exclusive access to the serial port. Then they can open the same port from multiple threads, relying on the OS's internal locking.

On *nix this is done with an ioctl, whereas on Windows we would set dwShareMode = FILE_SHARE_READ | FILE_SHARE_WRITE in the call to CreateFileW.

[nagisa]

I needed this (the split write/read ends) as well. I think the model where it is possible to split the read end from the write end is independently useful from the ability to clone the whole port entirely and caters to different sets of use-cases and has different downsides.

Note, that serial handles are inherently not thread safe, because the underlying hardware has state (the port configuration), unlike the usual sockets and file handles. The following "wants" are inherently incompatible and one will have to pick 2 out of 1) full-duplex communication; 2) configurable ports; 3) ability to share handle between threads.¹

To expand on those… to allow 1 and 2, the handle must be unique, making reading, writing and configuration state changes well ordered; to allow 2 and 3, some sort of locking must employed to ensure (re-)configuration does not invalidate in-flight I/O; to allow 1 and 3, port state must be eliminated or become constant.

I feel like an ideal API would encompass all these variants, but I don’t think it is likely that any single API will handle all these cases well.

---

So after reviewing the MSDN documentation I'm pretty confident that it is thread safe, even though it is not explicitly mentioned. This is because of the example it gave.

The majority of the APIs in Windows are thread-safe by default, apparently. To the point that they point out when API is not thread-safe only.

Footnotes

1. Multi-producer/Multi-consumer I/O falls under the same constraints as full-duplex communication. ↩

[Pavlos1]

I've done some more testing on this. Turns out Windows will not let you open a serial port twice, so try_clone is probably the way to go here. For now I'm using a workaround where I'm removing the Drop implementation and deriving Clone. (A proper implementation of course must be more nuanced than this as we saw above.)

@nagisa Windows and Linux both do OS-level locking on read/write operations to file handles, so I don't think changing the settings from another thread is a problem. In any case .try_clone() can just be made to fail on platforms that don't have MT-safe read/write/configure.

[nagisa]

Windows and Linux both do OS-level locking on read/write operations to file handles, so I don't think changing the settings from another thread is a problem.

I’m about 100% sure that on Windows changing settings even within the same thread is already a problem. See, on Windows writing to a serial port does not block – the data instead goes into an internal buffer of sorts, and doing something like this:

write(...);
SetCommState(...);

will likely end up switching the baudrate in the middle of the preceding write.

[Pavlos1]

on Windows writing to a serial port does not block

As I understand it, WriteFile will block unless the FILE_FLAG_OVERLAPPED flag is set. (In the current implementation of serial-rs it is not.)

Now, my point is undermined by the fact that this flag is required if we want to concurrently read and write to the same serial port.

I would argue, however, that protecting the user from such effects isn't the library's job. More concretely, we can say that changing the baud rate concurrently during a write will result in the bytes being transferred at an unspecified/variable baud rate—this would be consistent with the unix implementation as well.

If on the other hand doing so would crash the program or otherwise cause undefined behavior, then we wouldn't want to expose this functionality (or at least do so in unsafe functions). But as I understand it the Windows API functions are thread-safe in the sense that this doesn't happen.

[Pavlos1]

So I've decided to look at this with new eyes.

From my testing (see gist), it seems that POSIX will drop exclusive access to the serial port once all file descriptors are closed. This should mean that we can freely implement .try_clone() with dup() calls, and since the existing Drop trait implementation closes the fds anyway the exclusive access thing should sort itself out. (i.e. The crate should not need to do its own reference counting — let me know if I'm missing something here.)

Windows behaves similarly with DuplicateHandle — once all handles have been closed the file object is de-allocated & the serial port can be opened from other processes again.

I can make a PR implementing what I've written above if @dcuddeback agrees with this reasoning.

It should be noted, however, that Windows will serialize reads and writes if this approach is used to implement #40. We can get around that by using FILE_FLAG_OVERLAPPED as I had alluded to earlier, but then reads/writes become async and we would need to decide whether to expose that to the user.

Also: On POSIX having exclusive access to the serial port is not the default (the crate asks for it explicitly), whereas on Windows it is and you cannot override it. It might be a good idea to add that as a config option, though unfortunately it would need to throw errors at people attempting to use it from Windows.

P.S. It's not necro-posting if the issue is still open, right? :)