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

dhttp: A simple production-ready HTTP server library for the 2020s #6

Merged
merged 15 commits into from
Jan 29, 2021

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented Jan 28, 2021

A while back, because of a test flake that @thallgren noticed, I became aware of a bug in the x/net/http2/h2c library, which I subsequently submitted a fix for. However, while figuring out that bug, I became aware of a growing amount of things that were "wrong", in deep-seated architectural ways I couldn't just file a PR for. It became apparent to me that using *http.Server was simply unacceptable if you're doing h2c (cleartext HTTP/2... every one of our products does h2c all over the place), in a way that couldn't be fixed without changing the API.

So, I introduce *github.com/datawire/dlib/dhttp.ServerConfig, a replacement for *net/http.Server (it still ultimately uses an *http.Server internally, but that's an implementation detail).

The godoc has a lot better introduction/rationale.

Each of the TextXXX functions that I added corresponds to an actual bug in either Telepresence or AES or both.

To review

I suggest first reading the final godocs, to get the big picture. Then review the implementation commit-by-commit (if you even want to review the implementation).

Because of import-cycle reasons, I created derror, and moved PanicToError to there from dutil. This means that dutil now only exists for backward compatibility with older versions of dlib.

$ COLUMNS=80 git diff --stat lukeshu/misc HEAD -- :!*_test.go :!*_bundle.go                                                                                                                                                                               
 .circleci/.gitignore      |   1 +
 .circleci/bundle.d/go.mod |   5 +
 .circleci/bundle.d/go.sum |  26 ++++
 .circleci/bundle.d/pin.go |   5 +
 Makefile                  |   1 +
 derror/panic.go           |  81 ++++++++++
 dgroup/group.go           |   4 +-
 dhttp/Makefile            |  18 +++
 dhttp/server.go           | 371 ++++++++++++++++++++++++++++++++++++++++++++++
 dhttp/server_hijacked.go  |  75 ++++++++++
 dhttp/server_http2.go     |  65 ++++++++
 dutil/http_context.go     |  84 -----------
 dutil/http_server.go      | 132 +++++++++++++++++
 dutil/panic.go            |  80 +---------
 go.mod                    |   2 +-
 go.sum                    |  10 +-
 16 files changed, 796 insertions(+), 164 deletions(-)

@LukeShu LukeShu requested review from kflynn, thallgren and rhs and removed request for rhs January 28, 2021 16:13
Base automatically changed from lukeshu/misc to master January 28, 2021 16:30
Copy link
Contributor

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks!! My comments are pretty much entirely about docs and comments (shocking, I know) so let me preface the whole review by saying that I look forward to seeing Ambassador switch to using it. 🙂

Approving since everything I have can easily be addressed later.

dutil/http_server.go Outdated Show resolved Hide resolved
@@ -73,9 +73,9 @@ func ListenAndServeHTTPSWithContext(ctx context.Context, server *http.Server, ce
// server.Shutdown() when the soft Context is canceled, and the hard Context being canceled causes
// the .Shutdown() to hurry along and kill any live requests and return, instead of waiting for them
// to be completed gracefully.
func ServeHTTPWithContext(ctx context.Context, server *http.Server, listener net.Listener) error {
func ServeHTTPWithContext(ctx context.Context, server *http.Server, ln net.Listener) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes me sad that the code switched from listener to ln for the variable name, instead of the docs changing from ln to listener. 😂

dutil/http_server.go Outdated Show resolved Hide resolved
dhttp/server.go Show resolved Hide resolved
dhttp/server_hijacked.go Show resolved Hide resolved
dhttp/server.go Show resolved Hide resolved
dhttp/server.go Show resolved Hide resolved
dhttp/server.go Show resolved Hide resolved
dhttp/server.go Show resolved Hide resolved
dhttp/server.go Show resolved Hide resolved
Copy link
Contributor

@thallgren thallgren left a comment

Choose a reason for hiding this comment

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

Looks really good to me.

@LukeShu LukeShu merged commit 55ccb74 into master Jan 29, 2021
@LukeShu LukeShu deleted the lukeshu/http2 branch January 29, 2021 17:40
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.

3 participants