Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

RFC: TCP Backpressure hooks #1311

Closed
SeanTAllen opened this issue Oct 12, 2016 · 9 comments
Closed

RFC: TCP Backpressure hooks #1311

SeanTAllen opened this issue Oct 12, 2016 · 9 comments
Assignees

Comments

@SeanTAllen
Copy link
Member

Add support for TCP backpressure to Pony's TCPConnection actor.

https://github.com/ponylang/rfcs/blob/master/text/0017-tcp-backpressure.md

@SeanTAllen
Copy link
Member Author

I want to get this integrated into our code at work and no one has grabbed it, so, assigning myself.

@SeanTAllen SeanTAllen self-assigned this Oct 13, 2016
@SeanTAllen
Copy link
Member Author

SeanTAllen commented Oct 14, 2016

@jemc after giving it more consideration (as it was left open as part of the RFC), I think based on how we are using at Sendence that from a performance standpoint, having two notifier methods related to outgoing connections experiencing backpressure is the correct way to go. almost all our code does an if on status inside the

 fun ref throttled(conn: TCPConnection ref, status: Bool) =>
    """
    Called when there is a change in backpressure status of the connection
    """
    None

methods that we have.

I'm thinking throttled and released.

thoughts?

@SeanTAllen
Copy link
Member Author

SeanTAllen commented Oct 14, 2016

@jemc testing question as this RFC was before we implemented "how do we test this".

I'm thinking that for read backpressure hook testing, the test can be to set mute on a connection and then verify that within some period of time (a ponytest timeout) that received is not called on a receiver's notifier (and we should send data to said connection). a timeout is considering passing and received being called is grounds for a failure.

we test write backpressure by muting a receiving connection, trying to write to it and verifying that throttled is called on the sender's notifier. we then unmute the receiving connection and verify that released is called on the sender's notifier.

the write backpressure test here might need to be two tests.

thoughts?

@jemc
Copy link
Member

jemc commented Oct 14, 2016

Regarding splitting throttled into two methods - seems appropriate to me for the reasons you explained. However, I'm not necessarily wild about released - seems too vague, and almost sounds related to dispose at first glean. How would you feel about unthrottled?


Back before we had the RFC process I was working on implementing this feature, and having clean tests was the part that was holding me up. I was working on tests basically like what you're describing, so it sounds right to me.

@SeanTAllen
Copy link
Member Author

I like released when you know the context is "backpressure" but that isn't at all clear and I was really hesitant to use "release". I'm good with "unthrottled" although I wish we had something better.

I'm updating to "unthrottled"

@SeanTAllen
Copy link
Member Author

SeanTAllen commented Oct 16, 2016

@jemc this write test won't work...

we test write backpressure by muting a receiving connection, trying to write to it and verifying that throttled is called on the sender's notifier. we then unmute the receiving connection and verify that released is called on the sender's notifier.

the amount of data we need to send in order to get throttled is unknown. its based on OS buffer sizes. I'm not sure how to test throttling. thoughts?

we could send a ton of data to make sure we can overpower just about any buffer but that seems mighty problematic and not something i would want to do.

@jemc
Copy link
Member

jemc commented Oct 17, 2016

we could send a ton of data to make sure we can overpower just about any buffer but that seems mighty problematic

If we don't know exactly how much data it takes to throttle, could we try a strategy of sending data on the writer end until the throttled occurs?

@SeanTAllen
Copy link
Member Author

@jemc i think that would involve a really long timeout which could still be hit without triggering the condition. still seems flakey.

@SeanTAllen
Copy link
Member Author

This was resolved by #1322.

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

No branches or pull requests

2 participants