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

Initialization problems on Windows #1123 #1127

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

innova-engineering
Copy link

@innova-engineering innova-engineering commented Apr 29, 2019

This prevent pplx to initialize Winsock from a global object by initialize it manuell.
This is required since it creates problems if you call WSAStartup in DLLMain.
The fix I used is proposed in the winsock_init.hpp of boost asio.

@innova-engineering
Copy link
Author

I have no idea why some of the Pipelines fail.
My changes do only apply on windows, but pipelines on Ubuntu, Android and IOS fail.
After having a look to the log files it seems as there are some boost 1.69 problems on this platforms, that are unrelated to my changes.

@BillyONeal
Copy link
Member

Some of the Pipelines are broken at this time because they keep changing the Pipelines machines out from underneath us; your change doesn't look to be the cause of that.

That said, I'm a bit concerned because you have put an ifdef WIN32 inside an !defined(_WIN32) block.

Can you describe the problem you're trying to solve here more directly and why we don't see it in any form of testing? Why does the exact same solution already in use by winsock_init.hpp you mention not already resolve this problem?

@innova-engineering
Copy link
Author

innova-engineering commented May 13, 2019

Hey, sorry for the delayed reply. I somehow missed the notification on this one.

First off all yes, the inserted block is at the wrong position and should be moved out of the !defined(_WIN32) block. I will change this.

But let me describe the problem that i try to solve in more detail.
We included the cpprestsdk via vcpkg in our product.
This result in crashes on Windows 7 with the registry value CEIPEnable set to 1.
We tracked the issue down to calling WSAStartup(MAKEWORD(major, minor), &wsa_data) in DllMain, which is a bad idea as mentioned in the documentation:

The WSAStartup function typically leads to protocol-specific helper DLLs being loaded. As a result, the WSAStartup function should not be called from the DllMain function in a application DLL.

WSAStartup get called at the initialization of an static variable defined in asio/detail/winsock_init.hpp.
This header get included in the cpprestsdk source code via the #include "boost/asio.hpp" in threadpool.h.
Asio know about the initialization problems that can occur and inserted an mechanism to suppress the automatic initialization and do it manually.
The part that i inserted do suppress the automatic initialization.
To use WinSock you have to call WSAStartup manually now, but it seems for me that you do not use it, so I did not insert the manual initialization.
So far the problem description, I hope it is getting clear, what I tried to fix here.

An other way of fixing this issue would be to make sure that winsock_init.hpp does not get included in the code, but it is included via various headers of asio and do not know if it is possible to include the header you need without including winsock_init.hpp. And even if it is possible the problem could easily reappear if asio changes some includes internally.

@BillyONeal
Copy link
Member

If the problem is that you can't call WSAStartup inside DllMain, I can see how that might spell doom -- however, this change doesn't actually achieve that. The constructor for this manual_winsock_init variable runs in DllMain just as much as the constructor for the automatic version provided by asio.

@innova-engineering
Copy link
Author

innova-engineering commented May 13, 2019

Sorry, I should have mentioned what the manual_winsock_init does.
The initialization of winsock has a init count, so that the initialization only happens if it is not initialized yet.
manual_winsock_init increase the init count of winsock without calling WSAStartup. The automatic initialization does not happen than since it looks like WSAStartup was already called.

If you want to you can have a look at the implementation here:
winsock_init.hpp
winsock_init.ipp

@BillyONeal
Copy link
Member

I see. In that case how does WSAStartup get called?

Alexander Lehmann added 2 commits May 14, 2019 09:40
@msftclas
Copy link

msftclas commented May 14, 2019

CLA assistant check
All CLA requirements met.

fix winsock_init_helper - uppercase template arguments
@innova-engineering
Copy link
Author

Well yes, I missed the manual init.
I inserted that part now. I think it should look like this.

Fix static variable init
Fix ifdef
@innova-engineering
Copy link
Author

I think I have solved the issues with it. Any chance to get this merged?

@BillyONeal
Copy link
Member

Can you merge with origin/master?

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