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

Introduce TCP API #433

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Introduce TCP API #433

wants to merge 7 commits into from

Conversation

NikitaNikolaenko
Copy link
Contributor

@NikitaNikolaenko NikitaNikolaenko commented Jun 18, 2022

TODO:

  1. Documentation
  2. SSL tests (raw ssl, just using the openssl stuff)
  3. POSIX tests (currently we only have winapi tests)
  4. Combine IPv4 and IPv6 tests (but there's a problem with dazel rn, since it doesn't support IPv6)

@NikitaNikolaenko NikitaNikolaenko self-assigned this Jun 18, 2022
@NikitaNikolaenko NikitaNikolaenko force-pushed the folming.asio-tcp-2 branch 11 times, most recently from 101cf70 to d1d7041 Compare June 20, 2022 15:22
@CodingCanuck
Copy link
Contributor

This is a pretty huge review! It's going to be pretty tough to do a solid review on this and keep all of the details in my head at once. That not only will make it tricky to give/receive high quality review feedback with fast turnaround time, but it also increases the chance that this review lasts for a long time which could make it pretty difficult to get through.

Can we think about how to break this up into multiple smaller reviews that we can review and merge incrementally? (Here's a "why" guide for small PRs, but it doesn't touch much on the "how": https://google.github.io/eng-practices/review/developer/small-cls.html )

I haven't dug into this deeply enough to figure out: are there standalone files that can be reviewed independently that we could start with? Or some initial classes or function we could add first?

Context for me as a reviewer: I'm not familiar with the details of TCP, so it's even more valuable than normal to have small/digestible reviews since I'll need an unusual amount of time to gather the domain knowledge I'll need to review this properly.

One quick structural question: I see lots of files starting with the tcp- prefix. Should we instead move these into a tcp folder and drop the prefix (kind of like how we have a grpc folder)?

Copy link
Contributor

@CodingCanuck CodingCanuck left a comment

Choose a reason for hiding this comment

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

(Moving myself off of the action list while we think about ways to break this PR up into more reviewable chunks)

@NikitaNikolaenko
Copy link
Contributor Author

NikitaNikolaenko commented Jul 14, 2022

@benh, I've changed the behavior of our EventLoop. Now it is running asio::io_context inside libuv's uv_loop_t using uv_check_t. This way we can still use EventLoop::Schedule to schedule our eventuals that use asio::io_context.

The only part that I'm not really sure about is the part with uv_unref. Do we even need it?

@NikitaNikolaenko NikitaNikolaenko force-pushed the folming.asio-tcp-2 branch 4 times, most recently from ee19898 to 63beb98 Compare July 14, 2022 20:56
@NikitaNikolaenko NikitaNikolaenko force-pushed the folming.asio-tcp-2 branch 4 times, most recently from 6acbfa5 to d51f568 Compare July 29, 2022 13:28
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.

2 participants