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

connect: Add TCP Fast Open support for Windows #3378

Closed
wants to merge 1 commit into from
Closed

Conversation

@bagder
Copy link
Member

@bagder bagder commented Dec 16, 2018

This is the former PR #3327, saved by Johannes Schindelin (@dscho), rebased, squashed
and pushed again.

Requires Windows 10 ver 1607 or newer

This is the former PR #3327, saved by Johannes Schindelin, rebased, squashed
and pushed again.

Requires Windows 10 ver 1607 or newer
@bagder
Copy link
Member Author

@bagder bagder commented Dec 21, 2018

Thanks for the tumbs up. If you're also 👍 for actually merging this, I'm all ears. I personally cannot tell how the logic around the presence and use of this function is or should be!

Loading

@@ -609,6 +609,12 @@ Vista
# endif
#endif

/* Define if you have the ConnectEx WinSock 2 extension */
/* This requires a build target of WinXP or newer */
#if defined(_WIN32_WINNT) && (_WIN32_WINNT >= 0x0501)
Copy link
Member

@MarcelRaad MarcelRaad Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this here at all if it's overridden in curl_setup.h anyway?

Loading

Copy link
Member Author

@bagder bagder Dec 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The curl_setup.h logic doesn't define HAVE_CONNECTEX though so if we would remove this section the other one would need polish.

Loading

@dscho
Copy link
Contributor

@dscho dscho commented Dec 24, 2018

If you're also 👍 for actually merging this, I'm all ears. I personally cannot tell how the logic around the presence and use of this function is or should be!

It is definitely not ready for merging in my mind. I mentioned in the original PR how I think the runtime check should work: simply by verifying that the Curl_ConnextEx function pointer is non-NULL. If the function cannot be found when asked for, it is safe to assume that it is not supported on the current system. Of course, it would also be good to make sure that this runtime check is not performed over and over again on system that do not support ConnectEx(), so the patch really would need a bit more work (and testing!).

Loading

@bagder
Copy link
Member Author

@bagder bagder commented Jan 10, 2019

I slapped the "needs update" label on this to mark that its not ready. I personally am not the right person to drive through the changed mentioned above so I'll leave this open for a while to see if someone else wants to take it through to the goal or if it doesn't happen in another several weeks I'll probably close and add a mention in the TODO.

Loading

@bagder bagder closed this in a7c228a Feb 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2019
@bagder bagder deleted the TFO-windows branch Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants