the deprecated
CreateThread() instead of _beginthreadex()

CreateThread is not deprecated according to any documentation while _beginthreadex is more obscure, less popular and just calls CreateThread in the implementation.

which isn't recommended by
the documentation, except for "the most extreme cases"

TerminateThread hard aborts thread execution at the place it's executing at which can cause program crashes and state corruption. It should be avoided as much as possible.

the deprecated
CreateThread() instead of _beginthreadex()

CreateThread is not deprecated according to any documentation while _beginthreadex is more obscure, less popular and just calls CreateThread in the implementation.

Thanks, updated the PR message to not say deprecated.

I don't know if the preference for _beginthreadex() in curl was an
intentional or accidental choice. Whichever curl uses, it'd seem to
make sense to stick to one of them. Switching to CreateThread()
may make things simpler by aligning UWP with non-UWP. Would
there be any downside?

which isn't recommended by
the documentation, except for "the most extreme cases"

TerminateThread hard aborts thread execution at the place it's executing at which can cause program crashes and state corruption. It should be avoided as much as possible.

Do you see a way to perhaps drop that call from the curl tool code?

Would there be any downside?

Since _beginthreadex just calls CreateThread, I assume there'd be none, unless old VS versions (or maybe legacy CRT) had it implemented differently.
Still, CreateThread itself is supported since XP so doesn't seem like it'd loose any platfoms.

Do you see a way to perhaps drop that call from the curl tool code?

As far as I can see, it seems to only be called if the main thread fails to connect to a socket created by the console thread, would it not be possible to make the console thread connect to main thread instead and then have the thread creation be last in the code there so that there'd be no need to close stuff there?

The socket there could probably be replaced with CreateNamedPipe there too since AFAIR that's generally preferred for single machine communication.

Would there be any downside?

Since _beginthreadex just calls CreateThread, I assume there'd be none, unless old VS versions (or maybe legacy CRT) had it implemented differently. Still, CreateThread itself is supported since XP so doesn't seem like it'd loose any platfoms.

Pushed commits to switch to CreateThread(), let's see what CI has to say.

Do you see a way to perhaps drop that call from the curl tool code?

As far as I can see, it seems to only be called if the main thread fails to connect to a socket created by the console thread, would it not be possible to make the console thread connect to main thread instead and then have the thread creation be last in the code there so that there'd be no need to close stuff there?

Sounds sensible. Would you want to make a PR?

Sounds sensible. Would you want to make a PR?

I'm still working on the PR I mentioned earlier in the Windows XP PR (it's a Win32 async DNS resolver to replace the threaded one), I'd rather prefer to finish that before doing other stuff (doing it partially for the project I work on since we decided to abandon c-ares due to it being full of bugs and using the proper async APIs should be better than the threading hacks).

Sounds sensible. Would you want to make a PR?

I'm still working on the PR I mentioned earlier in the Windows XP PR (it's a Win32 async DNS resolver to replace the threaded one), I'd rather prefer to finish that before doing other stuff (doing it partially for the project I work on since we decided to abandon c-ares due to it being full of bugs and using the proper async APIs should be better than the threading hacks).

Got it, no worries!

/off The Win32 async DNS support reminded me of this commit (and those linked from the message), in case its useful reference: eb8ad66 #14794

@vszakats

I don't know if the preference for _beginthreadex() in curl was an
intentional of accidental choice

I guess for classic MinGW. It explicitly said to use _beginthreadex instead of CreateThread in its FAQ:
https://web.archive.org/web/20200611023910/https://mingw.org/wiki/Use_the_thread_library

@MarcelRaad Hm, I wonder if this may apply to non-old-mingw too. That
is, that _beginthreadex() is necessary when calling CRT functions from
the thread, and CreateThread() when not doing so. AFAICS in curl, before
this patch CreateThread()'s thread is also using the CRT.

https://learn.microsoft.com/en-us/cpp/parallel/multithreading-with-c-and-win32?view=msvc-170
says:
"If you call C run-time routines from a program built with libcmt.lib, you must start your threads with the _beginthread or _beginthreadex function. Do not use the Win32 functions ExitThread and CreateThread. Using SuspendThread can lead to a deadlock when more than one thread is blocked waiting for the suspended thread to complete its access to a C run-time data structure."

@MarcelRaad Hm, I wonder if this may apply to non-old-mingw too. That is, that _beginthreadex() is necessary when calling CRT functions from the thread, and CreateThread() when not doing so. AFAICS in curl, before this patch CreateThread()'s thread is also using the CRT.

https://learn.microsoft.com/en-us/cpp/parallel/multithreading-with-c-and-win32?view=msvc-170 says: "If you call C run-time routines from a program built with libcmt.lib, you must start your threads with the _beginthread or _beginthreadex function. Do not use the Win32 functions ExitThread and CreateThread. Using SuspendThread can lead to a deadlock when more than one thread is blocked waiting for the suspended thread to complete its access to a C run-time data structure."

I assume this only applies to the legacy CRT then, not the new UCRT.
I don't believe modern VS makes it possible to link against legacy CRT but I believe that Mingw-w64 still does it by default.

@MarcelRaad Hm, I wonder if this may apply to non-old-mingw too. That is, that _beginthreadex() is necessary when calling CRT functions from the thread, and CreateThread() when not doing so. AFAICS in curl, before this patch CreateThread()'s thread is also using the CRT.
https://learn.microsoft.com/en-us/cpp/parallel/multithreading-with-c-and-win32?view=msvc-170 says: "If you call C run-time routines from a program built with libcmt.lib, you must start your threads with the _beginthread or _beginthreadex function. Do not use the Win32 functions ExitThread and CreateThread. Using SuspendThread can lead to a deadlock when more than one thread is blocked waiting for the suspended thread to complete its access to a C run-time data structure."

I assume this only applies to the legacy CRT then, not the new UCRT. I don't believe modern VS makes it possible to link against legacy CRT but I believe that Mingw-w64 still does it by default.

So I guess if any one of the supported Windows targets still use or support non-UCRT
(which is the case), we must stick to _beingthreadex(). Also in that case, the
CreateThread() is wrong and should likely need to be fixed. _beingthreadex()
doesn't seem to have a TerminateThread() counterpart.

AFAIK VS2015 is the first version with UCRT support. We still stupport VS2008, soon
to be bumped to VS2010. mingw-w64 supports UCRT depending on configuration
and version. I'd personally wouldn't mind sticking with UCRT alone, but probably too
soon for that.

If the CRT can't initialize a thread created by CreateThread it terminates the thread, that is what it means by low memory conditions, it needs some memory to initialize the CRT when it is called since it isn't initialized beforehand since beginthreadex wasn't used. I think Windows probably does this anyway IIRC. I've worked in low memory and resource conditions and Windows will just kill things but (usually) the OS keeps running. Who's to say if beginthreadex could really do anything if you're out of memory. When you're out of memory all bets are off. Using CreateThread has one minor advantage which is it's easier to work when debugging because the start address will show as the function that was threaded and not the crt initialization wrapper.