FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-113812: Allow DatagramTransport.sendto to send empty data by ordinary-jamie · Pull Request #115199 · python/cpython · GitHub

/ cpython Public

gh-113812: Allow DatagramTransport.sendto to send empty data - #115199

Merged
gvanrossum merged 5 commits into
python:mainfrom
ordinary-jamie:asyncio-allow-empty-udp
Feb 17, 2024
Merged

gh-113812: Allow DatagramTransport.sendto to send empty data#115199
gvanrossum merged 5 commits into
python:mainfrom
ordinary-jamie:asyncio-allow-empty-udp

Conversation

ordinary-jamie commented Feb 9, 2024
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Update DatagramTransport.sendto method to not return when data is an empty bytes object. This allows users to send zero-length datagrams (used for example in Time Protocol RFC 868).


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

Copy link
Copy Markdown
Contributor Author

@gvanrossum -- This might cause an issue with the flow control of the transport. Since the buffer size is calculated with only the payload (and not the entire datagram).

The write buffer could in theory be flooded with zero-length datagrams, and the high watermark will never be crossed.

self._buffer_size += len(data)

Copy link
Copy Markdown
Member

@gvanrossum -- This might cause an issue with the flow control of the transport. Since the buffer size is calculated with only the payload (and not the entire datagram).

The write buffer could in theory be flooded with zero-length datagrams, and the high watermark will never be crossed.

Oh, that's a very good point. I think we could add a constant value to the "buffer size" for each packet added -- the only use for the buffer size is to interact with flow control. In fact, after skimming the UDP Wikipedia page, I think we can add 8 for each packet, since that's the protocol's header size.

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but let's do something about flow control first.

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LG -- I'll merge now!

Copy link
Copy Markdown
Member

Sorry, I'd like one more doc change. The code and docs are actually fine, but I feel this deserves a What's New entry (Doc/whatsnew/3.13.rst). A bullet in the existing asyncio section under "Improved Modules" should suffice. I'd like it to mention both the ability to send 0-length packets and the change to buffer size (since it can affect details around flow control). Probably a good idea to mention the latter in the news file too.

Copy link
Copy Markdown
Contributor Author

No worries :) Let me know what you think of this wording I just added!

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! Will merge later tonight.

ordinary-jamie commented Feb 17, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

Sorry Guido, just pushed a documentation change to for the version change note in the Documentation

Edit: Also pushed a typo fix (repeated "will now") sorry!

gvanrossum merged commit 73e8637 into python:main Feb 17, 2024

Copy link
Copy Markdown
Member

Thanks again, @ordinary-jamie!

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…ython#115199)

Also include the UDP packet header sizes (8 bytes per packet)
in the buffer size reported to the flow control subsystem.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…ython#115199)

Also include the UDP packet header sizes (8 bytes per packet)
in the buffer size reported to the flow control subsystem.
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
…ython#115199)

Also include the UDP packet header sizes (8 bytes per packet)
in the buffer size reported to the flow control subsystem.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL