Skip to content

Conversation

@gretchenfrage
Copy link
Collaborator

@gretchenfrage gretchenfrage commented May 11, 2025

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
Copy link
Member

djc commented May 11, 2025

Why is this better?

@gretchenfrage
Copy link
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.)

Copy link
Collaborator

@Ralith Ralith left a comment

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
20 checks passed
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