Skip to content

refactor(quinn): Remove some usage of execute_poll - #2232

Merged
Ralith merged 1 commit into
quinn-rs:mainfrom
gretchenfrage:call-execute-poll-less
May 14, 2025
Merged

refactor(quinn): Remove some usage of execute_poll#2232
Ralith merged 1 commit into
quinn-rs:mainfrom
gretchenfrage:call-execute-poll-less

Conversation

@gretchenfrage

@gretchenfrage gretchenfrage commented May 11, 2025

Copy link
Copy Markdown
Collaborator

This commit makes there be fewer places where SendStream::execute_poll is called directly. AsyncWrite implementations now go through poll_write, and poll_write now goes through the write future. This means that execute_poll is now only called directly from write and write_chunks.


Note: Cherry-picked from #2231.

@gretchenfrage
gretchenfrage requested review from Ralith and djc as code owners May 11, 2025 18:56
This commit makes there be fewer places where SendStream::execute_poll
is called directly. AsyncWrite implementations now go through poll_write,
and poll_write now goes through the write future. This means that
execute_poll is now only called directly from write and write_chunks.
@gretchenfrage
gretchenfrage force-pushed the call-execute-poll-less branch from d8cc4d3 to f6abb3a Compare May 11, 2025 18:57
@djc

djc commented May 11, 2025

Copy link
Copy Markdown
Member

Why is this better?

@gretchenfrage

Copy link
Copy Markdown
Collaborator Author

Why is this better?

It's simpler. Consider that the previous versions of these lines involve a closure, whereas the updated versions don't. The previous versions of the lines involve calling two of our custom methods (execute_poll and write), whereas the updated versions only call one (poll_write or write).

Previously, the logic of integrating execute_poll and write in an appropriate way existed at 4 duplicate sites, whereas with this PR it exists only in one site (write) and the other places becomes derivative of that (for example, in #2231 this makes it simpler to replace execute_poll entirely. But I'm only bringing up that draft PR for illustratory purposes--this refactor should stand on its own.)

@Ralith Ralith left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice to know that poll_write isn't actually primitive, though it may make sense to keep as a discoverable convenience.

@Ralith
Ralith added this pull request to the merge queue May 14, 2025
Merged via the queue into quinn-rs:main with commit 4f8a0f1 May 14, 2025
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.

3 participants