bpo-34932: Add socket.TCP_KEEPALIVE for macOS #25079

ShaneHarvey · 2021-03-29T21:08:42Z

This change adds socket.TCP_KEEPALIVE for macOS.

https://bugs.python.org/issue34932

corona10

Please update Doc/library/socket.rst first and I don't prefer to add TCP_KEEPALIVE as alias.

corona10 · 2021-03-30T14:43:01Z

Modules/socketmodule.c

    PyModule_AddIntMacro(m, TCP_KEEPIDLE);
+#elif defined(__APPLE__) && defined(TCP_KEEPALIVE)
+    /* TCP_KEEPALIVE is equivalent to TCP_KEEPIDLE on OSX. */
+    PyModule_AddIntConstant(m, "TCP_KEEPIDLE", TCP_KEEPALIVE);


Suggested change

PyModule_AddIntConstant(m, "TCP_KEEPIDLE", TCP_KEEPALIVE);

PyModule_AddIntMacro(m, TCP_KEEPALIVE);;

corona10 · 2021-03-30T14:43:37Z

Modules/socketmodule.c

    PyModule_AddIntMacro(m, TCP_KEEPIDLE);
+#elif defined(__APPLE__) && defined(TCP_KEEPALIVE)


Suggested change

#elif defined(__APPLE__) && defined(TCP_KEEPALIVE)

#if defined(TCP_KEEPALIVE)

bedevere-bot · 2021-03-30T14:44:04Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ShaneHarvey · 2021-03-30T17:06:51Z

I don't prefer to add TCP_KEEPALIVE as alias.

Noted. I've removed the alias and added TCP_KEEPALIVE directly.

ShaneHarvey · 2021-03-30T18:39:45Z

@corona10 Is there a reason you suggested to remove the defined(__APPLE__) condition? Without it the TCP_KEEPALIVE constant is added on windows which breaks this test:

FAIL: test_new_tcp_flags (test.test_socket.TestMSWindowsTCPFlags)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\lib\test\test_socket.py", line 6439, in test_new_tcp_flags
    self.assertEqual([], unknown,
AssertionError: Lists differ: [] != ['TCP_KEEPALIVE']

I have no idea what TCP_KEEPALIVE does on Windows. Should I:

keep socketmodule.c as is and update the test to expect TCP_KEEPALIVE on Windows, or:
change socketmodule.c to never add socket.TCP_KEEPALIVE on Windows

ShaneHarvey · 2021-04-21T18:44:03Z

After reading https://bugs.python.org/issue32394 to figure out why TestMSWindowsTCPFlags exists, I discovered that TCP_KEEPALIVE is undocumented on Windows:

https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-tcp-socket-options

Since we don't know what version of windows introduced this flag we cannot add it on Windows.

I've updated the code to only add TCP_KEEPALIVE on macOS (when it exists).

ShaneHarvey · 2021-04-21T18:44:26Z

I have made the requested changes; please review again.

bedevere-bot · 2021-04-21T18:44:28Z

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

ShaneHarvey · 2021-06-02T19:17:38Z

ping @corona10

ShaneHarvey · 2021-07-14T22:33:04Z

Rebased on to main.

ShaneHarvey · 2021-07-14T22:36:02Z

@corona10 @pablogsal I'm not sure what's holding up the review on this PR but I'm hoping it can be merged and backported to 3.10 similar to #25992.

miss-islington · 2021-07-14T22:53:17Z

Thanks @ShaneHarvey for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

(cherry picked from commit d59d737) Co-authored-by: Shane Harvey <shnhrv@gmail.com>

bedevere-bot · 2021-07-14T22:53:25Z

GH-27153 is a backport of this pull request to the 3.10 branch.

ShaneHarvey · 2021-07-14T23:01:56Z

Thanks all!

pablogsal · 2021-07-14T23:02:36Z

Thanks for your contribution!

(cherry picked from commit d59d737) Co-authored-by: Shane Harvey <shnhrv@gmail.com>

the-knights-who-say-ni added the CLA signed label Mar 29, 2021

bedevere-bot added the awaiting review label Mar 29, 2021

corona10 requested changes Mar 30, 2021

View reviewed changes

bedevere-bot removed the awaiting review label Mar 30, 2021

bedevere-bot added the awaiting changes label Mar 30, 2021

ShaneHarvey requested a review from corona10 March 30, 2021 17:06

ShaneHarvey changed the title ~~bpo-34932: Add socket.TCP_KEEPIDLE support to macOS~~ bpo-34932: Add socket.TCP_KEEPALIVE for macOS Mar 30, 2021

bedevere-bot removed the awaiting changes label Apr 21, 2021

bedevere-bot added the awaiting change review label Apr 21, 2021

ShaneHarvey and others added 6 commits July 14, 2021 15:31

bpo-34932: Add socket.TCP_KEEPIDLE support to macOS

4fc293e

📜🤖 Added by blurb_it.

4f804dc

bpo-34932: Add socket.TCP_KEEPALIVE for macOS

4c642ad

Add test case

026eb4b

TCP_KEEPALIVE is undocumented on Windows so only add it on macOS

856b137

Fix whitespace with 'make patchcheck'

b387cdf

ShaneHarvey force-pushed the issue34932 branch from 4be01bf to b387cdf Compare July 14, 2021 22:32

pablogsal approved these changes Jul 14, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 14, 2021

pablogsal added the needs backport to 3.10 only security fixes label Jul 14, 2021

pablogsal merged commit d59d737 into python:main Jul 14, 2021

bedevere-bot removed the awaiting merge label Jul 14, 2021

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 14, 2021

bpo-34932: Add socket.TCP_KEEPALIVE for macOS (pythonGH-25079)

ee2a82f

(cherry picked from commit d59d737) Co-authored-by: Shane Harvey <shnhrv@gmail.com>

bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 14, 2021

miss-islington added a commit that referenced this pull request Jul 14, 2021

bpo-34932: Add socket.TCP_KEEPALIVE for macOS (GH-25079)

ff7af22

(cherry picked from commit d59d737) Co-authored-by: Shane Harvey <shnhrv@gmail.com>

ShaneHarvey deleted the issue34932 branch July 15, 2021 03:16

pablogsal mentioned this pull request Jul 18, 2021

[3.10] Correct the order of regen-abidump #27228

Closed

	PyModule_AddIntConstant(m, "TCP_KEEPIDLE", TCP_KEEPALIVE);
	PyModule_AddIntMacro(m, TCP_KEEPALIVE);;

		PyModule_AddIntMacro(m, TCP_KEEPIDLE);
		#elif defined(__APPLE__) && defined(TCP_KEEPALIVE)

	#elif defined(__APPLE__) && defined(TCP_KEEPALIVE)
	#if defined(TCP_KEEPALIVE)

Uh oh!

bpo-34932: Add socket.TCP_KEEPALIVE for macOS #25079

bpo-34932: Add socket.TCP_KEEPALIVE for macOS #25079

Uh oh!

Conversation

ShaneHarvey commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 30, 2021

Uh oh!

ShaneHarvey commented Mar 30, 2021

Uh oh!

ShaneHarvey commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShaneHarvey commented Apr 21, 2021

Uh oh!

ShaneHarvey commented Apr 21, 2021

Uh oh!

bedevere-bot commented Apr 21, 2021

Uh oh!

ShaneHarvey commented Jun 2, 2021

Uh oh!

ShaneHarvey commented Jul 14, 2021

Uh oh!

ShaneHarvey commented Jul 14, 2021

Uh oh!

miss-islington commented Jul 14, 2021

Uh oh!

bedevere-bot commented Jul 14, 2021

Uh oh!

ShaneHarvey commented Jul 14, 2021

Uh oh!

pablogsal commented Jul 14, 2021

Uh oh!

Uh oh!

ShaneHarvey commented Mar 29, 2021 •

edited

Loading

ShaneHarvey commented Mar 30, 2021 •

edited

Loading