Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Windows TCP backpressure enhancements #4081

Closed
SeanTAllen opened this issue Apr 1, 2022 · 0 comments · Fixed by #4252
Closed

Windows TCP backpressure enhancements #4081

SeanTAllen opened this issue Apr 1, 2022 · 0 comments · Fixed by #4252
Assignees

Comments

@SeanTAllen
Copy link
Member

SeanTAllen commented Apr 1, 2022

When I put in the backpressure work in TCPConnection, I knew nothing about IOCP and Windows and as such, it is technically hooked up but may not be very effective or it might be overly effective. This issue is going to collect improvements that could be made:

pony_os_writev

For writing, in writev and write_final in TCPConnection the Windows, pony_os_writev is called. Currently on Windows, it always returns 0. However, pony_os_writev calls WSASend which handles all errors that aren't WSA_IO_PENDING as a fatal error and shuts down. I believe that WSAEWOULDBLOCK should be handled differently and if it is encountered, that is our signal to trigger backpressure more than _pending_sent length.

Moving away from _pending_sent would mean that we want to improve the logic in _complete_sent to not making release backpressure purely based on _pending_sent size.

pony_os_send

Also uses WSASend in the same way as pony_os_writev but it isn't called by anything in the standard library or runtime. We can improve the windows interface to indicate backpressure and document it.

iocp_send_to

iocp_send_to uses WSASendTo for datagrams which could also be told there is backpressure and could be adjusted as could its callers, however, we currently have no backpressure in the UDPsystem on POSIX either so this could reasonably be skipped for now and done as part of "backpressure for UDP" which... well, I'm not sure what that would look like. I've never considered backpressure for UDP.

@SeanTAllen SeanTAllen added help wanted Extra attention is needed enhancement labels Apr 1, 2022
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 1, 2022
@jemc jemc removed the discuss during sync Should be discussed during an upcoming sync label Apr 5, 2022
SeanTAllen added a commit that referenced this issue Nov 19, 2022
Our prior setting of backpressure for TCP writes on Windows was naive. It was
based purely on the number of buffers currently outstanding on an IOCP socket.
The amount of data didn't matter at all. Whether more data could be accepted or
not, also wasn't taken into consideration. This commit greatly improves the
situation by only applying backpressure when Windows tells us that it is
applying backpressure.

No more guessing.

Two runtime API methods have been updated on Windows.

The Windows version of `pony_os_writev` will now return the number of buffers
accepted or zero if backpressure is encountered. All other errors still cause
an error that must be handled on the Pony side of the API via a `try` block.

The Windows version of `pony_os_send` will now return the number of bytes
accepted or zero if backpressure is encountered. All other errors still cause
an error that must be handled on the Pony side of the API via a `try` block.

I consider these changes  non-breaking as previously, the return values from
both functions had no meaning.

Closes #4081
@SeanTAllen SeanTAllen removed the help wanted Extra attention is needed label Nov 19, 2022
@SeanTAllen SeanTAllen self-assigned this Nov 19, 2022
SeanTAllen added a commit that referenced this issue Nov 22, 2022
Our prior setting of backpressure for TCP writes on Windows was naive. It was
based purely on the number of buffers currently outstanding on an IOCP socket.
The amount of data didn't matter at all. Whether more data could be accepted or
not, also wasn't taken into consideration. This commit greatly improves the
situation by only applying backpressure when Windows tells us that it is
applying backpressure.

No more guessing.

Two runtime API methods have been updated on Windows.

The Windows version of `pony_os_writev` will now return the number of buffers
accepted or zero if backpressure is encountered. All other errors still cause
an error that must be handled on the Pony side of the API via a `try` block.

The Windows version of `pony_os_send` will now return the number of bytes
accepted or zero if backpressure is encountered. All other errors still cause
an error that must be handled on the Pony side of the API via a `try` block.

I consider these changes  non-breaking as previously, the return values from
both functions had no meaning.

Closes #4081
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants