The patch is fine, though zooming out, the use of TerminateThread() is strongly not recommended by the documentation. It's also the single place where this is done in the codebase:
https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminatethread#remarks

(Up until recently, all other places used the CRT API for thread creation, and that API doesn't have a terminate API.)

Earlier discussion: #18451
Initial commit: 9a26633 #17572

edit: Meant to say: If there is a way, this error handling should probably be solved differently, without killing the thread.

If there is a way, this error handling should probably be solved differently, without killing the thread.

It certainly sounds so, but I don't think I am the person to do that as it probably should involve running tests on Windows to work out what works and what doesn't work. It's not clear to me what the alternative is.

I don't think we need to terminate the thread on error, it looks like it is written in a way that it would abort on its own? @denandz Though I notice that there's similar cleanup in both the main thread and the worker thread and that doesn't make a lot of sense to me. Couldn't it lead to handles being closed twice etc if the worker thread has started but there is an error in the main thread?

main thread cleanup of tdata, sockets,etc:

worker thread cleanup of tdata, sockets,etc :

Should the worker thread really be doing any of that?

That block in the main thread is cleaning up if there's any issue spawning the worker and its various handles. The in-worker-cleanup closes the socket handles after the worker main processing loop is completed.

IIRC without the in-worker cleanup, I had the test suite catching unclosed-sockets on exit. The other bit was we need to call the socket close (along with the SO_DONTLINGER socket option) to avoid a case where Windows holds the socket in TIMEWAIT after curl exits (because we didn't close the socket) and subsequent execution of curl -T . fails until the kernel cleans up that socket.

Regarding TerminateThread - understood it's not recommended by Microsoft - the idea here was that if there's an issue during worker setup and win32_stdin_read_thread is going to return an error, we want to be sure there are no dangling threads kicking around and thus take the nuclear option to terminate the thread prior to returning error.

This could all probably be done in a neater way - like passing the tdata structure ref back to the main curl process and having it do all of the cleanup as part of exiting.

Would it be workable to setup the sockets first, and only create the thread if those succeeded?

Would it be workable to setup the sockets first, and only create the thread if those succeeded?

Possibly - the connect() blocks in the main thread with the worker doing the accept() - so there would need to be some trickery to have connect() and accept() happen in the same main thread, then get loaded into the tdata struct and the thread spawned.

I think there's two areas for improvement here - Moving away from TerminateThread and having the main thread explicitly clean up the sockets and handles used by the worker thread on exit.

Did we want to track this in a separate issue? I just tested this branch on a Windows host and the additional CloseHandle and global variable cleanup works fine.

Did we want to track this in a separate issue? I just tested this branch on a Windows host and the additional CloseHandle and global variable cleanup works fine.

Yes, I think we should do it in a separate PR to keep it clean. I can merge this as-is in the mean time.

Re: TerminateThread, here are some Raymond Chen articles about it:

https://devblogs.microsoft.com/oldnewthing/20150814-00/?p=91811
https://devblogs.microsoft.com/oldnewthing/20191101-00/?p=103046
https://devblogs.microsoft.com/oldnewthing/20140808-00/?p=293

/via https://news.ycombinator.com/item?id=45645320

Some more I found:

https://devblogs.microsoft.com/oldnewthing/20180727-00/?p=99355
https://devblogs.microsoft.com/oldnewthing/20070503-00/?p=27003
https://devblogs.microsoft.com/oldnewthing/20181228-00/?p=100585