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

Use overlapped interface with NamedPipe #17

Merged
merged 6 commits into from Aug 24, 2018

Conversation

cjubb39
Copy link
Contributor

@cjubb39 cjubb39 commented Aug 24, 2018

This pull request has two primary changes:

  1. Using GetOverlappedResult instead of lpNumberOfBytes{Read,Written} in ReadFile and WriteFile for read_overlapped and write_overlapped, respectively. According to https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-readfile and https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-writefile, if the file was opened with FILE_FLAG_OVERLAPPED,

The lpNumberOfBytesWritten parameter should be set to NULL. To get the number of bytes written, use the GetOverlappedResult function.

  1. Noting that all NamedPipes are created with FILE_FLAG_OVERLAPPED, their read and write implementations should use the lpOverlapped parameter. Create a variant that will wait for the completion before returning to satisfy the simple read and write interface.

This seemed the simplest change that didn't break the interface defaults but am open to suggestions / alternate solutions that use the overlapped interface with handles opened using FILE_FLAG_OVERLAPPED.

@alexcrichton
Copy link
Collaborator

Thanks! The only suggestion I'd have is that for the blocking read/write impls could this perhaps use a thread local cached even to avoid recreating one on each call?

@cjubb39
Copy link
Contributor Author

cjubb39 commented Aug 24, 2018

good suggestion -- I've made it use a threadlocal!

@alexcrichton alexcrichton merged commit a9f3b3c into yoshuawuyts:master Aug 24, 2018
@alexcrichton
Copy link
Collaborator

Thanks!

@cjubb39
Copy link
Contributor Author

cjubb39 commented Aug 24, 2018

Do you mind cutting a new release with this included when you get a chance? Not a huge rush for me :)

@alexcrichton
Copy link
Collaborator

Sure thing, done now!

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.

None yet

2 participants