gh-72894: Dynamically allocate select fd_sets on Windows based on inputs size by andzn · Pull Request #13842 · python/cpython

andzn · Jun 5, 2019

https://bugs.python.org/issue28708

Issue: Low FD_SETSIZE limit on Windows #72894

asvetlov · Jun 5, 2019

By this PR you basically increased the memory consumption for every select call from 12 KiB to 384 KiB.
I believe the number is too high.

See the issue's comment added recently

jdemeyer

To test the new seqsize() code path, I would like to see a test added where select.select() is called with an iterable which is not a tuple or list, for example iter([fd]) instead of [fd].

jdemeyer

You are also missing error checks where you call seqsize().

bedevere-bot · Jun 12, 2019

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.

And if you don't make the requested changes, you will be put in the comfy chair!

andzn · Jun 14, 2019

I have made the requested changes; please review again

bedevere-bot · Jun 14, 2019

Thanks for making the requested changes!

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

andzn · Jun 14, 2019

@jdmeyer I removed the seqsize function completely and replaced calls with PySequence_Length.

NewUserHa · Jun 15, 2019

hope this pass soon.
select() limit really is pain ass.

csabella · Jan 25, 2020

@andzn, please resolve the merge conflict. Thank you!

andzn · Jan 27, 2020

@csabella , merge conflicts are now resolved.

andzn · Jul 28, 2020

@asvetlov, @jdemeyer can you please take a look again at the PR? The requested changes have already been implemented a year ago.

arhadthedev · Feb 17, 2023

@andzn Could you sign the new CLA by clicking not signed button in the cpython-cla-bot's message, please?

andzn · Feb 17, 2023

@andzn Could you sign the new CLA by clicking not signed button in the cpython-cla-bot's message, please?

@arhadthedev done.

vstinner · Apr 5, 2023

+    do {
+        if (((fd_set FAR *)(set))->fd_count < setsize)
+            ((fd_set FAR *)(set))->fd_array[((fd_set FAR *)(set))->fd_count++]=(fd);
+    } while(0);


What is the purpose of the surrounding code do { ... } while (0)?

vstinner · Apr 5, 2023

+    size_t i;
+    for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {


Suggested change

size_t i;

for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

for (size_t i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

vstinner · Apr 5, 2023

@@ -196,7 +211,7 @@ set2list(fd_set *set, pylist fd2obj[FD_SETSIZE + 1])
        return NULL;

    i = 0;


Can you declare i here?

vstinner · Apr 5, 2023

    int n;
    _PyTime_t timeout, deadline = 0;

+    setsize = FD_SETSIZE;


I would prefer to initialize the variable where it's declared. Either declare it at this line, or move the declaration here. So the variable is never uninitialized.

vstinner · Apr 5, 2023

+        return NULL;
+    }
+
+    if ((size_t)rsize > setsize) setsize = (size_t)rsize;


You may write: setsize = Py_MAX(setsize, (size_t)rsize); (same for the 2 lines below). IMO it's more explicit (and how I would write it in Python).

vstinner · Apr 5, 2023

+    ofdset = (fd_set*) PyMem_NEW(SOCKET, setsize + 1);
+    efdset = (fd_set*) PyMem_NEW(SOCKET, setsize + 1);
+    if (ifdset->fd_array == NULL || ofdset->fd_array == NULL || efdset->fd_array == NULL) {
+        if (ifdset->fd_array) PyMem_DEL(ifdset->fd_array);


PyMem_DEL() is a alias to PyMem_Free(): use it directly.

The NULL test is useless and can be removed. PyMem_Free(NULL) is well defined, it does nothing: https://docs.python.org/dev/c-api/memory.html#c.PyMem_Free

vstinner · Apr 5, 2023

+    fd_set *ifdset, *ofdset, *efdset;
+#if !defined(MS_WINDOWS)
+    fd_set ifdset_stack, ofdset_stack, efdset_stack;
+#if !defined(SELECT_USES_HEAP)


It would make the code simpler if the SELECT_USES_HEAP macro would always be defined on Windows, no?

github-actions · Mar 4, 2026

This PR is stale because it has been open for 30 days with no activity.

Raise FD_SETSIZE limit to 16384

b1ea111

the-knights-who-say-ni added the CLA signed label Jun 5, 2019

bedevere-bot added the awaiting review label Jun 5, 2019

andzn changed the title ~~bpo-28708: Raise FD_SETSIZE limit to 16384~~ bpo-28708: Raise FD_SETSIZE limit to 16384 on windows Jun 5, 2019

Dynamically allocate select fd_sets on Windows based on inputs list size

c2e6ef0

andzn changed the title ~~bpo-28708: Raise FD_SETSIZE limit to 16384 on windows~~ bpo-28708: Dynamically allocate select fd_sets on Windows based on inputs size Jun 10, 2019

andzn added 3 commits June 10, 2019 16:44

Move new FD_SET macro definition under defined(MS_WINDOWS)

c19f865

Added News file

185db02

Fix linux & doc build

555f0e1

jdemeyer requested changes Jun 12, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jun 12, 2019

jdemeyer requested changes Jun 12, 2019

View reviewed changes

asvetlov requested changes Jun 12, 2019

View reviewed changes

Comment thread Modules/selectmodule.c Outdated

Comment thread Modules/selectmodule.c Outdated

Comment thread Modules/selectmodule.c

bedevere-bot added awaiting changes and removed awaiting core review labels Jun 12, 2019

Add back SELECT_USES_HEAP

35885cb

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 14, 2019

vstinner mentioned this pull request Jun 14, 2019

[WIP] bpo-28708: Add setsize keywoard-only parameter to select.select() #13845

Closed

csabella requested review from asvetlov and jdemeyer January 25, 2020 20:16

Merge branch 'master' into fix-issue-28708

e278594

dhirschfeld mentioned this pull request Jan 25, 2021

Choosing the Right ASGI Server for Deploying FastAPI fastapi/fastapi#2062

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into fix-issue-28708

003b808

arhadthedev changed the title ~~bpo-28708: Dynamically allocate select fd_sets on Windows based on inputs size~~ gh-72894: Dynamically allocate select fd_sets on Windows based on inputs size Feb 17, 2023

arhadthedev added performance Performance or resource usage OS-windows extension-modules C modules in the Modules dir topic-IO labels Feb 17, 2023

vstinner reviewed Apr 5, 2023

View reviewed changes

python deleted a comment Apr 7, 2025

github-actions Bot added the stale Stale PR or inactive for long period of time. label Mar 4, 2026

		size_t i;
		for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

	size_t i;
	for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {
	for (size_t i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

andzn commented Jun 5, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Jun 5, 2019

Uh oh!

jdemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

jdemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 12, 2019

Uh oh!

andzn commented Jun 14, 2019

Uh oh!

bedevere-bot commented Jun 14, 2019

Uh oh!

andzn commented Jun 14, 2019

Uh oh!

NewUserHa commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csabella commented Jan 25, 2020

Uh oh!

andzn commented Jan 27, 2020

Uh oh!

andzn commented Jul 28, 2020

Uh oh!

arhadthedev commented Feb 17, 2023

Uh oh!

andzn commented Feb 17, 2023

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

andzn commented Jun 5, 2019 •

edited by bedevere-bot

Loading

NewUserHa commented Jun 15, 2019 •

edited

Loading