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

Add SSL tests; Fix SSL bugs; Port over TCP improvements from Wallaroo #3172

Closed
wants to merge 5 commits into from

Conversation

dipinhora
Copy link
Contributor

This PR includes a number of TCP and SSL fixes/improvements (if desired, I can break them apart into separate PRs):

  • Cleanup TCPConnection GC-safety mechanism for writev buffers
    Prior to this commit, the TCPConnection pending_writev buffer had to
    have a secondary buffer called _pending to ensure that the data
    wouldn't get GC'd too early by the runtime.

    This commit removes that hack and replaces it with a slightly different
    hack based on the strategy used in File in PR Fix GC-safety issue with writev pointers in File. #2775. The hacky part
    is that now we have two buffers for writev data. One for windows
    (_pending_windows) and one for non-windows (_pending_posix) to
    account for the difference in order of the struct between windows
    and posix. This, however, seems less bad than the previous hack of
    having the secondary buffer to ensure GC safety.

    resolves Pointer._offset is private, but Array.cpointer takes an offset argument. #2782
    resolves Clean up TCPConnection GC-safety mechanism for writev buffers. #2779

  • Make reading via TCPConnection re-entrant safe
    Port over bc6f78bcaad55156393753c540befe9f581065bc from wallaroo.

    There's a nasty possible bug in TCPConnection before
    applying this change.

    Because _mute and _unmute are synchronous methods on
    TCPConnection and because they can be called inside
    _pending_reads via code executing in notify.received, its
    possible to mute and unmute a source while executing
    _pending_reads.

    Prior to this change, if you were to unmute from inside a pending
    reads via the above scenario, _unmute would synchronously call
    _pending_reads to re-enter that function and start processing
    data out of order. Further, we could end up in an infinite loop
    so long as enough data existed.

  • Change how TCP connection reads data to improve performance
    Port over of the "faster tcp" logic from wallaroo PRs:

    Update TCP Source and Data Channel WallarooLabs/wally#2203
    Update _pending_reads TCP logic WallarooLabs/wally#2563

    That provides a significant performance improvement to the
    throughputs and latencies for the wallaro performance test.

  • Make TCPConnection yield on writes to not hog cpu
    Port over of the following PR from wallaroo:

    Add _write_again behavior to TCP actors with _pending_writes WallarooLabs/wally#2565

    This commit adds a _write_again to the TCPConnection actor
    and modifies the _pending_writes function to yield by calling
    _write_again after having sent at least _max_size bytes.
    This will allow other actors and GC to run when a large amount
    of data has been queued up to be sent.

  • Add SSL tests and fix some SSL related bugs
    This commit adds SSL tests for expect, writev, mute, unmute, and
    throttle. As part of adding these tests, some bugs were encountered
    and fixed to get the tests to pass.

Prior to this commit, the TCPConnection `pending_writev` buffer had to
have a secondary buffer called `_pending` to ensure that the data
wouldn't get GC'd too early by the runtime.

This commit removes that hack and replaces it with a slightly different
hack based on the strategy used in `File` in PR ponylang#2775. The hacky part
is that now we have two buffers for `writev` data. One for windows
(`_pending_windows`) and one for non-windows (`_pending_posix`) to
account for the difference in order of the struct between windows
and posix. This, however, seems less bad than the previous hack of
having the secondary buffer to ensure GC safety.

resolves ponylang#2782
resolves ponylang#2779
Port over bc6f78bcaad55156393753c540befe9f581065bc from wallaroo.

There's a nasty possible bug in TCPConnection before
applying this change.

Because `_mute` and `_unmute` are synchronous methods on
TCPConnection and because they can be called inside
`_pending_reads` via code executing in `notify.received`, its
possible to mute and unmute a source while executing
`_pending_reads`.

Prior to this change, if you were to unmute from inside a pending
reads via the above scenario, `_unmute` would synchronously call
`_pending_reads` to re-enter that function and start processing
data out of order. Further, we could end up in an infinite loop
so long as enough data existed.
Port over of the "faster tcp" logic from wallaroo PRs:

WallarooLabs/wally#2203
WallarooLabs/wally#2563

That provides a significant performance improvement to the
throughputs and latencies for the wallaro performance test.
Port over of the following PR from wallaroo:

WallarooLabs/wally#2565

This commit adds a `_write_again` to the `TCPConnection` actor
and modifies the `_pending_writes` function to yield by calling
`_write_again` after having sent at least `_max_size` bytes.
This will allow other actors and GC to run when a large amount
of data has been queued up to be sent.
@dipinhora dipinhora requested a review from SeanTAllen June 8, 2019 20:02
This commit adds SSL tests for expect, writev, mute, unmute, and
throttle. As part of adding these tests, some bugs were encountered
and fixed to get the tests to pass.
@SeanTAllen
Copy link
Member

@dipinhora can you break apart into separate PRs and add CHANEGLOG entries for those to the PRs after opening them?

let data =
if _expect == 0 then
let data' = _read_buf = recover Array[U8] end
data'.truncate(_read_buf_offset)
Copy link
Member

Choose a reason for hiding this comment

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

this is a "bug" i just fixed in lori. see ponylang/lori@c61b165

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I noticed that "bug" but was trying not to do more than a "port" of the code as part of this.

@@ -758,12 +759,12 @@ actor TCPConnection
_next_size = _max_size.min(_next_size * 2)
end

Copy link
Member

Choose a reason for hiding this comment

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

this change is non-windows only, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, there is no logical change to the windows code. The only change to the windows code is to migrate from _read_len to _read_buf_offset.

@dipinhora
Copy link
Contributor Author

@SeanTAllen I've opened (adding changelog entries soon):

#3174
#3175
#3176
#3177

The PR for the faster tcp performance related change will have to wait for the TCP re-entrant change to be merged first.

@dipinhora
Copy link
Contributor Author

@SeanTAllen changelog entries added. But it seems that my adding [skip ci] to the changelog commits might not be ideal because it prevents the travis/circleci/appveyor tests from running.

@dipinhora dipinhora closed this Jun 9, 2019
@SeanTAllen
Copy link
Member

Yeah, you'd need to try and rewrite that commit to not have [skip ci]

@dipinhora
Copy link
Contributor Author

yay! let me change that and force push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants