Skip to content

Conversation

@stephen-hansen
Copy link
Contributor

@stephen-hansen stephen-hansen commented Dec 28, 2024

Just doing some cleanup of the socketserver docs regarding class attributes as mentioned in the linked issue.

  • Move address_family attribute from instance attributes to class attributes.
  • Document allow_reuse_port.
  • Document max_packet_size (note UDP only).
  • Add note to request_queue_size to denote TCP only.

📚 Documentation preview 📚: https://cpython-previews--128320.org.readthedocs.build/

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Thanks for doing this :)

Copy link

@reactive-firewall reactive-firewall left a comment

Choose a reason for hiding this comment

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

👍🏻 LGTM! With conditions (see knit-picky inline comment)

🙇 Thanks @stephen-hansen; These document drafts have already been useful to me.

Comment on lines +311 to +314
The maximum amount of data (in bytes) to receive at once. If a packet is
too large for the buffer, then the excess bytes beyond :attr:`max_packet_size`
are discarded. The default value is usually ``8192``, but this can be
overridden by subclasses.

Choose a reason for hiding this comment

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

LGTM! With conditions:

Please consider clarifying if the term "data" here, means without IP/UDP headers with something like:

Suggested change
The maximum amount of data (in bytes) to receive at once. If a packet is
too large for the buffer, then the excess bytes beyond :attr:`max_packet_size`
are discarded. The default value is usually ``8192``, but this can be
overridden by subclasses.
The maximum amount of data (in bytes) to receive at once. If a packet's
payload (excluding headers) is too large for the buffer, then the excess
bytes beyond :attr:`max_packet_size` are discarded. The default value
is usually ``8192``, but this can be overridden by subclasses.

or if this really is the literal maximum packet size:

Suggested change
The maximum amount of data (in bytes) to receive at once. If a packet is
too large for the buffer, then the excess bytes beyond :attr:`max_packet_size`
are discarded. The default value is usually ``8192``, but this can be
overridden by subclasses.
The maximum amount of data (in bytes) to receive at once. If a packet's
payload (including headers) is too large for the buffer, then the excess
bytes beyond :attr:`max_packet_size` are discarded. The default value
is usually ``8192``, but this can be overridden by subclasses.

Rational:
Omission of any mention of header when talking about a "packet" being "too large" is somewhat misleading to the reader.
not sure if going further is relevant to the python side, but for convenience sake I'll mention, the typical 20-byte IPV4 header (RFC-791 gives 20-byte base header, 40-byte maximum options) and the required 8-byte header assuming UDP (RFC-768).

Hopefully this helps.


Additional considerations (TL;DR):

  • MTU, and fragmentation boundaries are possible reasons to want to change the default (eg. expected max burst count * max packet size (including headers) on network path with values of 5 * (1500 - 20 - 40 - 8) = 7160 so the default is somewhat larger than the similar stream sockets' request_queue_size default)
  • UDP max packet size (after headers) is a possible (albeit unlikely) reason for increasing the size well beyond the default of 8192

@hugovk hugovk removed the needs backport to 3.12 only security fixes label Apr 10, 2025
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review docs Documentation in the Doc dir needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes skip news

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

6 participants