GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-113812: Allow DatagramTransport.sendto to send empty data by ordinary-jamie · Pull Request #115199 · python/cpython

Conversation

@ordinary-jamie

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

Uh oh!

There was an error while loading. Please reload this page.

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).

  • Issue: asyncio DatagramTransport.sendto does not send empty UDP packets #113812

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ordinary-jamie
Allow DatagramTransport.sendto to send empty data
67d6bed
@ordinary-jamie
ordinary-jamie requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners February 9, 2024 05:01
@bedevere-app bedevere-app Bot added the awaiting review label Feb 9, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 9, 2024
asyncio DatagramTransport.sendto does not send empty UDP packets #113812
Closed
@ordinary-jamie

ordinary-jamie commented Feb 9, 2024

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.

cpython/Lib/asyncio/selector_events.py

Line 1279 in 67d6bed

self._buffer_size += len(data)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Feb 16, 2024

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.

ordinary-jamie reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum reviewed Feb 16, 2024
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ordinary-jamie
Add header bytes to buffer size
1bcea07
@ordinary-jamie
ordinary-jamie requested a review from gvanrossum February 16, 2024 22:57
gvanrossum
gvanrossum approved these changes Feb 17, 2024
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LG -- I'll merge now!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 17, 2024
@gvanrossum

gvanrossum commented Feb 17, 2024

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.

ordinary-jamie and synodriver reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ordinary-jamie
Update news
1a01d17
@ordinary-jamie

ordinary-jamie commented Feb 17, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ordinary-jamie
ordinary-jamie requested a review from gvanrossum February 17, 2024 02:09
gvanrossum
gvanrossum approved these changes Feb 17, 2024
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks! Will merge later tonight.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ordinary-jamie
Update documentation
9e4f4a7
@ordinary-jamie

ordinary-jamie commented Feb 17, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ordinary-jamie
typos
0d2a757
gvanrossum
gvanrossum approved these changes Feb 17, 2024
View reviewed changes
@gvanrossum
gvanrossum merged commit 73e8637 into python:main Feb 17, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 17, 2024
@gvanrossum

gvanrossum commented Feb 17, 2024

Copy link
Copy Markdown
Member

Thanks again, @ordinary-jamie!

ordinary-jamie reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@ordinary-jamie @woodruffw
pythongh-113812: Allow DatagramTransport.sendto to send empty data (p…
177fa26
…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
@ordinary-jamie @diegorusso
pythongh-113812: Allow DatagramTransport.sendto to send empty data (p…
215b8d5
…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
@ordinary-jamie @LukasWoodtli
pythongh-113812: Allow DatagramTransport.sendto to send empty data (p…
e7d4cb6
…ython#115199)

Also include the UDP packet header sizes (8 bytes per packet)
in the buffer size reported to the flow control subsystem.
@kumaraditya303 kumaraditya303 mentioned this pull request Jul 26, 2025
3.13 asyncio DatagramTransport buffer size accounting regression. #135444
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
@willingc willingc Awaiting requested review from willingc willingc is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@ordinary-jamie @gvanrossum
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.