Skip to content

Comments

Windows 10 compatibility#11

Open
crystalgreen wants to merge 10 commits intomicrosoft:dc_9xfrom
crystalgreen:dc_9x
Open

Windows 10 compatibility#11
crystalgreen wants to merge 10 commits intomicrosoft:dc_9xfrom
crystalgreen:dc_9x

Conversation

@crystalgreen
Copy link

Hello again, here my 2nd pull request for the same changes.
Neither the release 9.0.0 not the latest branch worked for me on Windows 10. I started investigating and finally found a fix, along with some other minor fixes (see commit messages). Maybe I should have made multiple pull requests.

The main issue I discovered was the following. When a child process is created, it fails to duplicate the socket with the incoming request.
This throws a 10038 socket error.
The reason is that the function WSADuplicateSocket is called in the child process but according to its doc
it must be called from the parent process.
This sample shows how to do it correctly. They used named pipes to transfer the socket infos to the child process. This is not very elegant as this could cause conflicts with the pipe name. So I used STDIN to communicate from parent to child, as is explained here.
It seems Windows 10 is more strict about socket duplication as it works on older Windows systems.
I tested my code on a Windows 7 PC and it worked there too.

CBA added 9 commits March 24, 2017 15:05
…in child but must be done in parent.

The child threw socket error 10038. Doc of WSADuplicateSocket states it must be called in parent process.
See also (here)[https://memset.wordpress.com/2010/10/13/win32-api-passing-socket-with-ipc-method/] on how to do it correctly and (here)[https://msdn.microsoft.com/en-us/library/windows/desktop/ms682499(v=vs.85).aspx] on how to use STDIN to talk to child process.
… ensure it works when path contains spaces.
Previously, it was not passed if port=6311. This led to use of the port 7004, the default port of Rserve.exe.
(Why is the default port no longer 6311 anyway?)
…nto dc_9x

# Conflicts:
#	Rserve/src/Rserv.c
@crystalgreen
Copy link
Author

I will sign the CLA shortly, probably by end of next week.

@dec100
Copy link
Contributor

dec100 commented May 10, 2017

In your code where you setup the pipes. I believe the SetHandleInformation for stdout should be:

if (!SetHandleInformation(hChildStd_OUT_Rd, HANDLE_FLAG_INHERIT, 0))
return -1;
where you are reading from stdout

Also, is it possible to run a "unix2dos" on your Rserv.c source file and resubmit? The diff on the file shows everything being replaced.

Thanks!

@crystalgreen
Copy link
Author

You are right with your remark concerning the pipe setup. I just committed a fix. Thanks.

The line endings is related to git. I use the git setting core.autocrlf=true which keeps local CRLF but commits as LF. You probably see LF because you have it set to false. True is the recommended setting on Windows, see also here.

Concerning the CLA it may take a while as my boss wants to ask his boss first as this question has not yet occurred to us yet. But I don't see why this shouldn't pass.

@swells
Copy link
Contributor

swells commented Jul 18, 2017

@crystalgreen

Is it possible to resubmit your PR based off of master rather than the Microsoft:dc_9x branch?

Thanks for your help.

@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@swells swells self-requested a review October 17, 2017 16:32
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.

5 participants