Simplify buffering in TCPConnection #3185

SeanTAllen · 2019-06-18T03:39:33Z

Prior to this commit, the buffering in TCPConnection was overly
complicated. This was a carry over from the old style of buffer
handling. However, after Dipin Hora's commits to bring over the "faster
tcp" changes from Wallaroo, the buffer handling no longer makes sense.

Prior to this commit, a TCPConnection would be provided two variables to
control read buffering: init_size, max_size. The read buffer would start
at init_size and would grow to max_size.

Before "faster tcp", the buffer would only grow in size if we read the
entire buffer's worth of data in one recv call. After the data was read,
the buffer would be truncated and then handed off to the
TCPConnectionNotify. This was very wasteful. If you had a buffer of 128
allocated bytes and only read 4, then the 124 allocated bytes would be
truncated away and the buffer would be reallocated.

With "faster tcp", we allocate a large buffer and when reading data, we
chop it off the buffer as needed. This means, that "grow the buffer"
doesn't make much sense anymore. The buffer will continually shrink in
size as we slowly chop more and more off. This meant that with the code
as it is prior to change, we would always grow to max buffer size.

If we are always going to grow to max buffer size, it makes sense to
simplify the code and our interfaces and only allow for setting the
buffer size. This commit makes that simplication. Now we will:

allocate a buffer of size read_buffer_size
reallocate when the buffer is empty
reallocate if the caller is using expect and the buffer is too small
to hold the expected payload

This is a breaking change for TPCConnection and TCPListener as it
changes the constructor interfaces.

SeanTAllen · 2019-06-18T03:48:10Z

packages/net/tcp_connection.pony

@@ -657,7 +648,7 @@ actor TCPConnection
      var bytes_to_send: USize = 0
      var bytes_sent: USize = 0
      while _writeable and (_pending_writev_total > 0) do
-        if bytes_sent >= _max_size then
+        if bytes_sent >= _read_buffer_size then


I opened an issue #3186 to address this issue. I think it should be a different PR.

SeanTAllen · 2019-06-18T03:49:06Z

packages/net/tcp_connection.pony

@@ -775,16 +766,13 @@ actor TCPConnection
    This occurs only with IOCP on Windows.
    """
    ifdef windows then
-      match len.usize()
-      | 0 =>
+      if len.usize() == 0 then


seems like this usize() probably isn't needed but I wanted to do the minimal change.

SeanTAllen · 2019-06-18T03:50:28Z

packages/net/tcp_connection.pony

@@ -879,8 +861,8 @@ actor TCPConnection
            end
          end

-          if sum >= _max_size then
-            // If we've read _max_size, yield and read again later.
+          if sum >= _read_buffer_size then


we should consider freeing "yield after" from buffer size in which case #3186 should be updated.

jemc · 2019-06-18T16:12:32Z

Can you please add a link to PR #3178 in the commit and PR description?

Prior to this commit, the buffering in TCPConnection was overly complicated. This was a carry over from the old style of buffer handling. However, after Dipin Hora's commits to bring over the "faster tcp" changes from Wallaroo, the buffer handling no longer makes sense. Prior to this commit, a TCPConnection would be provided two variables to control read buffering: init_size, max_size. The read buffer would start at init_size and would grow to max_size. Before "faster tcp", the buffer would only grow in size if we read the entire buffer's worth of data in one recv call. After the data was read, the buffer would be truncated and then handed off to the TCPConnectionNotify. This was very wasteful. If you had a buffer of 128 allocated bytes and only read 4, then the 124 allocated bytes would be truncated away and the buffer would be reallocated. With "faster tcp", we allocate a large buffer and when reading data, we chop it off the buffer as needed. This means, that "grow the buffer" doesn't make much sense anymore. The buffer will continually shrink in size as we slowly chop more and more off. This meant that with the code as it is prior to change, we would always grow to max buffer size. If we are always going to grow to max buffer size, it makes sense to simplify the code and our interfaces and only allow for setting the buffer size. This commit makes that simplication. Now we will: - allocate a buffer of size `read_buffer_size` - reallocate when the buffer is empty - reallocate if the caller is using `expect` and the buffer is too small to hold the expected payload This is a breaking change for TPCConnection and TCPListener as it changes the constructor interfaces. Additional information on the "faster tcp" change can be found in the following PR: #3178

SeanTAllen · 2019-06-18T17:00:42Z

@jemc comment amened.

dipinhora · 2019-06-18T22:39:21Z

@SeanTAllen it seems like this change cannot handle the scenario when _expect > _read_buffer_size (or it can but i'm not following the logic correctly).

Can you please clarify/confirm?

SeanTAllen · 2019-06-18T22:41:47Z

Correct. Expect is capped at whatever the buffer size is.

SeanTAllen · 2019-06-18T22:43:26Z

And I forgot to open a PR for that change to expect.

dipinhora · 2019-06-18T22:44:13Z

That seems like a significant functional change beyond the "simplify buffering" as the title of this PR suggests.

SeanTAllen · 2019-06-18T22:46:22Z

I thought I had already opened a PR to address expect

dipinhora · 2019-06-18T22:49:46Z

Okey dokey. I'll wait to see that PR. Thanks.

SeanTAllen · 2019-06-18T22:50:44Z

The core change there is

  fun ref expect(qty: USize = 0) ?=>
    """
    A `received` call on the notifier must contain exactly `qty` bytes. If
    `qty` is zero, the call can contain any amount of data. This has no effect
    if called in the `sent` notifier callback.
    """
    if qty <= _read_buffer_size then
      if not _in_sent then
        _expect = _notify.expect(this, qty)
        _read_buf_size()
      end
    else
      error
    end

Prior to this commit, `expect` could be used to create a denial of service on a Pony application. If you used expect to handle a framed protocol, then, you'd by default accept any sized `except` value which could result in huge amounts of memory being allocated. Memory that could vastly outstrip the max buffer size set for controlling memory usage. After this commit, `expect` is partial and will result in an error if attempting to set an expect value that is greater than our max read buffer size. I intended to commit this prior to another PR, but forgot and thought that I had. As it is, this PR works in conjunction with #3185 to prevent expect from being passed that is greater than the max buffer size thereby deadlocking the logic post #3185.

SeanTAllen · 2019-06-18T23:08:15Z

@dipinhora #3197

Prior to this commit, `expect` could be used to create a denial of service on a Pony application. If you used expect to handle a framed protocol, then, you'd by default accept any sized `except` value which could result in huge amounts of memory being allocated. Memory that could vastly outstrip the max buffer size set for controlling memory usage. After this commit, `expect` is partial and will result in an error if attempting to set an expect value that is greater than our max read buffer size. I intended to commit this prior to another PR, but forgot and thought that I had. As it is, this PR works in conjunction with #3185 to prevent expect from being passed that is greater than the max buffer size thereby deadlocking the logic post #3185.

Prior to this commit, `expect` could be used to create a denial of service on a Pony application. If you used expect to handle a framed protocol, then, you'd by default accept any sized `except` value which could result in huge amounts of memory being allocated. Memory that could vastly outstrip the max buffer size set for controlling memory usage. After this commit, `expect` is partial and will result in an error if attempting to set an expect value that is greater than our max read buffer size. I intended to commit this prior to another PR, but forgot and thought that I had. As it is, this PR works in conjunction with ponylang#3185 to prevent expect from being passed that is greater than the max buffer size thereby deadlocking the logic post ponylang#3185.

SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Jun 18, 2019

SeanTAllen force-pushed the single-buffer-size-tcp branch from 74ec837 to 68a82a3 Compare June 18, 2019 03:40

SeanTAllen mentioned this pull request Jun 18, 2019

TCPConnection read/write buffer sizes are using the same variable #3186

Closed

SeanTAllen commented Jun 18, 2019

View reviewed changes

jemc approved these changes Jun 18, 2019

View reviewed changes

SeanTAllen force-pushed the single-buffer-size-tcp branch from 68a82a3 to 2f9fdb2 Compare June 18, 2019 17:00

SeanTAllen merged commit 2167fe3 into master Jun 18, 2019

SeanTAllen deleted the single-buffer-size-tcp branch June 18, 2019 17:03

SeanTAllen mentioned this pull request Jun 18, 2019

Close Denial of Service issue with TCPConnection.expect #3197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify buffering in TCPConnection #3185

Simplify buffering in TCPConnection #3185

SeanTAllen commented Jun 18, 2019

SeanTAllen Jun 18, 2019

SeanTAllen Jun 18, 2019

SeanTAllen Jun 18, 2019

jemc commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

dipinhora commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

dipinhora commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

dipinhora commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

Simplify buffering in TCPConnection #3185

Simplify buffering in TCPConnection #3185

Conversation

SeanTAllen commented Jun 18, 2019

SeanTAllen Jun 18, 2019

Choose a reason for hiding this comment

SeanTAllen Jun 18, 2019

Choose a reason for hiding this comment

SeanTAllen Jun 18, 2019

Choose a reason for hiding this comment

jemc commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

dipinhora commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

dipinhora commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

dipinhora commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019

SeanTAllen commented Jun 18, 2019