socket lib beahavior change in 3.6.4 #76575
Comments
|
On Windows, python 3.6.3 code "hasattr(socket, 'TCP_KEEPCNT')" gives False, python 3.6.4 gives True. This behavior breaks many libraries that i use. |
skn78
mannequin
added
stdlib
dtdev@dtdev-centos $ python3
Python 3.6.3 (default, Oct 11 2017, 18:17:01)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-18)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> hasattr(socket, 'TCP_KEEPCNT')
True
>>> I have linux system. Above is the output i got. |
|
Which libraries break? And which version of Windows are you running on? If TCP_KEEPCNT was only added recently (perhaps in an SDK update) then it may not really be available on all versions. (+Ned for awareness of the regression - not sure we need to respin immediately, but let's figure it out asap) |
|
It's a compile-time option in socketmodule.c. https://github.com/python/cpython/blob/3.6/Modules/socketmodule.c#L7466 The MSDN page suggests that it was added for Win10: https://msdn.microsoft.com/en-us/library/windows/desktop/ms738596(v=vs.85).aspx Is it possible that the build machine has changed its OS/SDK between |
|
Yeah, I updated the build machine before doing 3.6.4rc1 (I never update between rc and final releases). I'm intrigued to see why it breaks libraries though - typically unsupported enum values like this are silently ignored on older Windows versions. |
|
My OS version is Windows 7 x64. import socket
import platformprint('1) OS Info: ', platform.architecture(), platform.platform()) Result for python 3.6.3:
Result for python 3.6.4:
|
|
Yes, I recognise that the change happened. I don't understand what breaks as a result. You said it breaks "many libraries" - can you name some of them and provide sample code? |
|
websocket-client 0.44.0 My script gives the following Erroe: File "C:\Program Files\Python36\lib\site-packages\websocket\_http.py", line 108, in _open_socket |
|
Okay, so it looks like we don't have any prior art to conditionally omit constants from _socket based on OS level, and nothing in the setsockopt() doc suggests that it may fail like this. So we either need to explicitly exclude this symbol on Windows (at least until we drop support for pre-Windows 10 versions) or silently ignore setsockopt errors for future good arguments. I'm inclined to do the former - other opinions? |
|
Excluding for now seems like a simple option. (Basically a reversion to Messing with setsockopt seems a little more risky. In short: I'm with you -- exclude for now. |
|
I suggest inserting the following code into socket.py: if hasattr(sys, 'getwindowsversion') and sys.getwindowsversion()[0] < 10:
del socket.TCP_KEEPCNT
del socket.TCP_FASTOPEN |
|
Steve, is there something to be done here for 3.7.0? |
|
I like Kamil's suggestion. |
|
Base on Kamil's code, a little improvements. if hasattr(sys, 'getwindowsversion'):
WIN_MAJOR, _, WIN_BUILD, *_ = sys.getwindowsversion()
if WIN_MAJOR == 10:
if WIN_BUILD >= 15063: # Windows 10 1703
pass
elif WIN_BUILD >= 14393: # Windows 10 1607
del socket.TCP_KEEPCNT
else:
del socket.TCP_KEEPCNT
del socket.TCP_FASTOPEN
elif WIN_MAJOR < 10:
del socket.TCP_KEEPCNT
del socket.TCP_FASTOPENWindows 10 versions: |
|
Nice. Think you can turn that into a pull request on GitHub? |
|
So we either need to explicitly exclude this symbol on Windows (at least until we drop support for pre-Windows 10 versions) or silently ignore setsockopt errors for future good arguments. I'm inclined to do the former - other opinions? Imagine someone changed those values in his/her code, for example: His/her code works fine on new version Windows. Will this line break things again on old version Windows? |
|
Yes, adding the member back will put you back in the broken state, but there's nothing we can do to stop it. Thanks for the PR - I'll take a look, and if I'm able to log into GitHub on my phone maybe finish it. |
|
PR looks good to me. Unfortunately without SMS (I'm travelling) I can't get into my account from my phone, so if someone else wants to add the backport tags (3.7 and 3.6) and finish it then feel free. |
|
Glad to see PR 5468 not merged, I found it makes socket.py complicated. Now I'm inclined to patch the code in PyInit__socket(void) of socketmodule.c There already has a MS-Windows version checking In there, we can use GetVersionEx instead of GetVersion to get BuildNumber, then delete unusable opinions. (I don't have enough skill to modify .c code) |
|
I suggest inserting the following code into socketmodule.c: CHANGE:
#ifdef TCP_KEEPCNT
PyModule_AddIntMacro(m, TCP_KEEPCNT);
#endif
TO:
#ifdef TCP_KEEPCNT
#if defined(_MSC_VER) && _MSC_VER >= 1800
// Windows 10 1703 (15063)
if (IsWindows10CreatorsOrGreater()) {
PyModule_AddIntMacro(m, TCP_KEEPCNT);
}
#endif
#endif
AND CHANGE:
#ifdef TCP_FASTOPEN
PyModule_AddIntMacro(m, TCP_FASTOPEN);
#endif
TO:
#ifdef TCP_FASTOPEN
#if defined(_MSC_VER) && _MSC_VER >= 1800
// Windows 10 1703 (Build: 14393)
if (IsWindows10AnniversaryOrGreater()) {
PyModule_AddIntMacro(m, TCP_FASTOPEN);
}
#endif
#endif |
With сorrect comments:
CHANGE:
#ifdef TCP_KEEPCNT
PyModule_AddIntMacro(m, TCP_KEEPCNT);
#endif
TO:
#ifdef TCP_KEEPCNT
#if defined(_MSC_VER) && _MSC_VER >= 1800
if (IsWindows10CreatorsOrGreater()) { //Windows 10 1703(Build:15063 )
PyModule_AddIntMacro(m, TCP_KEEPCNT);
}
#endif
#endif
AND CHANGE:
#ifdef TCP_FASTOPEN
PyModule_AddIntMacro(m, TCP_FASTOPEN);
#endif
TO:
#ifdef TCP_FASTOPEN
#if defined(_MSC_VER) && _MSC_VER >= 1800
if (IsWindows10AnniversaryOrGreater()) { //Windows 10 1607(Build: 14393)
PyModule_AddIntMacro(m, TCP_FASTOPEN);
}
#endif
#endif |
I am sorry, this is the right version
CHANGE:
#ifdef TCP_KEEPCNT
PyModule_AddIntMacro(m, TCP_KEEPCNT);
#endif
TO:
#ifdef TCP_KEEPCNT
#ifdef MS_WINDOWS
#if defined(_MSC_VER) && _MSC_VER >= 1800
//on Windows avaible only from Windows 10 1703 (Build:15063 )
if (IsWindows10CreatorsOrGreater()) {
PyModule_AddIntMacro(m, TCP_KEEPCNT);
}
#else
PyModule_AddIntMacro(m, TCP_KEEPCNT);
#endif
#endif
AND CHANGE:
#ifdef TCP_FASTOPEN
PyModule_AddIntMacro(m, TCP_FASTOPEN);
#endif
TO:
#ifdef TCP_FASTOPEN
#ifdef MS_WINDOWS
#if defined(_MSC_VER) && _MSC_VER >= 1800
//on Windows avaible only from Windows 10 1607(Build: 14393)
if (IsWindows10AnniversaryOrGreater()) {
PyModule_AddIntMacro(m, TCP_FASTOPEN);
}
#else
PyModule_AddIntMacro(m, TCP_FASTOPEN);
#endif
#endif |
|
I create a new one (PR 5523), I'm not a C & socket expert, so if you want to improve/polish the patch, feel free to create a new PR based on (or not) it. |
|
We don't remove unsupported socket flags on Unix, why should we do it for Windows? |
|
Socket constants a compile time values, obviously concrete operation system might not support a flag -- but we do nothing with it in runtime. All flags available on compile time are exposed, it's true for modules like socket, os, select etc. |
|
The other option would be to always hide the new constant on Windows in 3.6, and make it unconditionally available on 3.7. |
We have this problem because: compile with new Windows SDK, but run on old version Windows.
Search on GitHub [1], most people only check whether Most of they don't check platform or Python version, so I'm -1 on this option. ----------------- [1] https://github.com/search?l=Python&p=1&q=TCP_KEEPCNT&type=Code&utf8=%E2%9C%93 |
No. Compile-time and run-time system is not always consist. There are some cases that system may return error for unsupported setsockopt() on Linux. So I think websocket-client should catch OSError for setsockopt. But if there are many broken libraries in the world, it's considerable Kamil said "This behavior breaks many libraries that i use." |
|
It seems Linux has TCP_KEEPCNT from very old ages and |
|
What's about other OS/flags? Or the issue is specific for TCP_KEEPCNT for Windows only? |
I agree with you. It almost impossible.
As far as my understanding, Yes. This issue is only for it. |
|
I definitely don't think we should get into the game of trying to guess which flags are supported at runtime and only exposing those. It's not as simple as keeping a table of OS versions -- which would be hard enough to get right -- but on Linux you can have things like vendor backports of features to old versions, or a new kernel that happens to have had a particular feature configured out of it. (For example, AFAIK some major cloud providers still use kernels that have had IPv6 support removed entirely.) |
Four flags involved. keyword available result 3.6 branch is using 1703 SDK, 3.7/3.8 branches are using 1709 SDK. This discussion is beyond my knowledge, I keep watch. |
|
I suggest closing the issue as "won't fix": checking in runtime for only for TCP flags on Windows is a weird exception. Client libraries should process such situations themself. |
|
I'm sympathetic to the idea that we don't want to carry around these checks, but also to the idea that this caused a regression in a micro release and that's not cool. Hence the idea that maybe we should keep everything the way it is in 3.7, but disable the new constants in 3.6, so that it arrives as part of a major release. OTOH it's been in 3.6 for like 6 weeks already so I guess a lot of the affected parties are already working around it. |
|
If the fix will land into 3.6 bugfix release only -- I can live with it, while the overall looks tricky. Ned Deily, what do you think about? |
I would like to have Steve's opinion. |
|
In this case I like the flags disappearing on older versions, just as they would if you built CPython on a version of Linux that didn't have the flags. The problem is that the Windows SDK always defines enum values for all Windows versions even if you are targeting an older version, as most APIs silently ignore unknown flags. Since there's no way to reliably remove the flags at build time, it'll have to be done at import time. In Python, existence normally implies availability, so we should maintain that here, especially since this API raises errors with these flags. |
|
Oh, and checking Windows version is hard. Better for us to get it right than every single library to risk getting it wrong. (The code used in the second PR is not right - there are IsWindowsVersion* macros that are better.) |
|
Ok |
|
Here is PR 5585 for 3.6 branch. For 3.7+, I would suggest patch in socketmodule.c like this: PyMODINIT_FUNC
PyInit__socket(void)
{
PyObject *m, *has_ipv6;
...
...
...
+#ifdef MS_WINDOWS
+ return remove_unusable_flags(m);
+#else
return m;
+#endif
}In this way, we handle the flags in a separated function remove_unusable_flags(m). Timelines FYI: 3.6.5 candidate: 2018-03-12 (tenative) 3.7.0 beta 2: 2018-02-26
I have an idea about this concern, I will post it after some experiments. |
Let alone run-time check. Flags only depend on .C code and the building SDK, therefore, for a certain official release (e.g. CPython 3.6.5), the flags are fixed. I wrote a script to dump/compare these flags in some Windows related modules (written in C language), see attached file winsdk_watchdog.py. Let me demonstrate how to use it:
Comparing official 3.6.3 (1607 SDK) with official 3.6.4 (1703 SDK).
Comparing official 3.6.4 (1703 SDK) with official 3.7.0b1 (1709 SDK). _winapi added 15 constants, after searching on GitHub repository, we know they were added in 2 commits: socket added 3 constants. mmap added 1 constants. This check is only needed after switching to a newer Windows SDK. As the file name, it's a watchdog of Windows SDK. Some people build third-party-build by themselves, it's also possible to create a unittest, and teach it how to recognize flexible-flags (may be removed during run-time). For example, a man builds CPython 3.7.0b1 with 1607 SDK (official 3.7.0b1 build with 1709 SDK), then he got a prompt like this:
If he build CPython 3.7.0b1 with 1903 SDK in two years later, he may got prompt like this:
|
|
What's the status of this issue? 3.7.0b2 is tagging in 48 hours or so and 3.6.5rc1 is in less than 2 weeks. |
|
I think PRs could be merged |
|
Agreed. I've merged these (and Miss Islington should get the 3.7 backport when CI completes) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: