Skip to content

Fix #168 - Windows: Support timeout on Interface::control_in and control_out#169

Open
piersfinlayson wants to merge 1 commit intokevinmehall:mainfrom
piersfinlayson:main
Open

Fix #168 - Windows: Support timeout on Interface::control_in and control_out#169
piersfinlayson wants to merge 1 commit intokevinmehall:mainfrom
piersfinlayson:main

Conversation

@piersfinlayson
Copy link

Per #168.

As well as potentially setting the timeout with a new (Windows only) method on Device, it is also arguable that control_in and control_out should reset the timeout to 5000ms after the transfer.

@martinling
Copy link
Contributor

This seems like a good idea - it's annoying not to be able to control the timeout on Windows.

Note that there is a race here in the case where the same Device or Interface is in use by two threads simultaneously, i.e.:

  1. Thread A sets timeout to X for request P
  2. Thread B sets timeout to Y for request Q
  3. Thread A issues request P, which now has the wrong timeout (Y instead of X).
  4. Thread B issues request Q

Of course, having two threads making control requests to the same device/interface simultaneously is an odd thing to do, and would almost certainly to lead to other problems anyway, but I figured it was still worth pointing this out.

If we care, we could avoid this possibility by having the code hold a lock whilst setting the timeout and submitting the transfer.

Another possible improvement: the current timeout value could be stashed (e.g. in InterfaceState), and the SetPipePolicy call skipped if the required timeout is already set. That would save a lot of unnecessary calls in the case where the same timeout is used repeatedly, which is common in practice.

@kevinmehall
Copy link
Owner

Yes, the reason I didn't implement it like this is because of the race condition @martinling mentioned. It's not clear whether WinUsb_SetPipePolicy from multiple threads and/or with another transfer pending on the endpoint is safe. PIPE_TRANSFER_TIMEOUT seems a lot less likely to be dangerous than RAW_IO in previous discussions, but I can't find any documentation about the thread safety of that function.

The way I was considering doing this is like the Linux implementation: set our own timer and cancel the transfer after the timeout. This would allow the timeouts of each transfer to be set independently like on the other platforms. I was looking at using CreateThreadpoolTimer for a Windows-provided timer without needing another timer thread or dependency.

A lock around the entire transfer would be another way to be able to use WinUSB's timeout safely, but it would have to be semi-custom to work with either async or blocking waits and return a MaybeFuture.

@piersfinlayson
Copy link
Author

@kevinmehall Thanks for the input. Are you're planning a CreateThreadpoolTimer version and do you have a rough ETA on that? Need to decide what to do with my dependent crate in the meantime.

@kevinmehall
Copy link
Owner

Work in progress on #171

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.

3 participants