Skip to content

Fix the default write buffer low-water mark - #778

Open
Shaisolaris wants to merge 1 commit into
MagicStack:masterfrom
Shaisolaris:default-low-water-mark
Open

Shaisolaris wants to merge 1 commit into
MagicStack:masterfrom
Shaisolaris:default-low-water-mark

Conversation

@Shaisolaris

Copy link
Copy Markdown

UVBaseTransport.__cinit__ sets the initial write buffer limits to

self._high_water = FLOW_CONTROL_HIGH_WATER * 1024
self._low_water = FLOW_CONTROL_HIGH_WATER // 4

FLOW_CONTROL_HIGH_WATER is in KiB (64), so the low-water mark starts at 16 bytes instead of 16 KiB. vanilla asyncio starts at (16384, 65536), and uvloop itself returns (16384, 65536) after set_write_buffer_limits() is called with no arguments, because add_flowcontrol_defaults() multiplies by 1024 before dividing.

Reproducer:

import asyncio, uvloop

async def main():
    loop = asyncio.get_running_loop()
    srv = await loop.create_server(asyncio.Protocol, '127.0.0.1', 0)
    port = srv.sockets[0].getsockname()[1]
    tr, _ = await loop.create_connection(asyncio.Protocol, '127.0.0.1', port)
    print(tr.get_write_buffer_limits())

uvloop.run(main())

On main this prints (16, 65536) for TCP, and the same for streams and write pipes; asyncio prints (16384, 65536). In practice a protocol that gets pause_writing() is only resumed once the buffer is almost fully drained, rather than at 16 KiB as with asyncio.

The change adds the missing * 1024, and adds an assertion on the initial limits to test_flowcontrol_mixin_set_write_limits.

Tested on Linux x86_64, CPython 3.12:

  • tests/test_tcp.py -k flowcontrol_mixin_set_write_limits: fails before the change ((16, 65536) != (16384, 65536)), passes after.
  • tests/test_tcp.py tests/test_pipes.py tests/test_unix.py: 141 passed, 21 skipped after the change.

I did not run the full test suite, macOS, or other Python versions.

UVBaseTransport set its initial low-water mark to
FLOW_CONTROL_HIGH_WATER // 4, which is 16 bytes, while the high-water
mark is FLOW_CONTROL_HIGH_WATER * 1024. asyncio and
set_write_buffer_limits() both use high // 4, which is 16 KiB.
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.

1 participant