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

GH-93316: socketmodule: remove constraint on queue size for macOS and FreeBSD. #93315

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

devnexen
Copy link
Contributor

For those, it does make senseso the queue is set to SOMAXCONN (128 on
macOS desktop maybe more on the server flavor ?).

@devnexen devnexen changed the title socketmodule: remove constraint on queue size for macOS and FreeBSD. GH-93316: socketmodule: remove constraint on queue size for macOS and FreeBSD. May 28, 2022
@gvanrossum
Copy link
Member

The code looks fine, the question is whether we should even keep the < 0 check for other platforms.

It would also be nice to have a test.

@ronaldoussoren
Copy link
Contributor

ronaldoussoren commented May 30, 2022

The Single Unix Specification mentions that a negative backlog should be interpreted as 0: https://pubs.opengroup.org/onlinepubs/9699919799/functions/listen.html

That makes the current check unnecessary and save to remove (and also means that the stated behaviour on macOS and FreeBSD is non-compliant).

@devnexen : Do you have a reference that documents this behaviour of those to platforms? The manpage for listen doesn't mention this.

UPDATE: My earlier comment about non-compliance is incorrect, the spec says that < 0 should be interpreted as == 0, and == 0 may update the backlog to some implementation defined minimum value. That's what's happening here.

@devnexen
Copy link
Contributor Author

The Single Unix Specification mentions that a negative backlog should be interpreted as 0: https://pubs.opengroup.org/onlinepubs/9699919799/functions/listen.html

That makes the current check unnecessary and save to remove (and also means that the stated behaviour on macOS and FreeBSD is non-compliant).

@devnexen : Do you have a reference that documents this behaviour of those to platforms? The manpage for listen doesn't mention this.

macOs mentions subtly in its manpage (128 max but that's macOs desktop) however confirmed in the libc https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/bsd/kern/uipc_socket.c#L1061
FreeBSD mentioned more clearly it s ok to use a negative backlog.

@devnexen
Copy link
Contributor Author

The code looks fine, the question is whether we should even keep the < 0 check for other platforms.

It would also be nice to have a test.

There is an existing test testing negative backlog.

@ronaldoussoren
Copy link
Contributor

I don't think a change is needed here, at least not for macOS. The code you linked to sets the backlog to SOMAXCONN when the value is <= 0. Python's listen binding doesn't change that (we clamp negative values to 0, but that's still setting the actual backlog to SOMAXCONN).

Just dropping the whole block of code would simplify the code base a little, but I'm not sure if it is worth the trouble.

@devnexen
Copy link
Contributor Author

I don't think a change is needed here, at least not for macOS. The code you linked to sets the backlog to SOMAXCONN when the value is <= 0. Python's listen binding doesn't change that (we clamp negative values to 0, but that's still setting the actual backlog to SOMAXCONN).

Just dropping the whole block of code would simplify the code base a little, but I'm not sure if it is worth the trouble.

So the BSD conforms and I did the change and triggered the tests on linux.

Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

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

Could you add tests that explicitly use listen(0) and listen(-1) and create a connection afterwards?

On first glance we don't have such tests as the moment and adding such a test would quickly show if one of the supported platforms misbehaves with a negative backlog.

@bedevere-bot
Copy link

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.

devnexen and others added 4 commits June 17, 2022 12:31
For those, it does make senseso the queue is set to SOMAXCONN (128 on
 macOS desktop maybe more on the server flavor ?).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants