http3: handle ErrAbortHandler when the handler panics #3575

shade34321 · 2022-10-07T17:34:07Z

Adds in the ErrAbortHandler and checks for later as the stdlib does. Not sure if this is complete though.

google-cla · 2022-10-07T17:34:11Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

marten-seemann · 2022-10-07T17:50:39Z

http3/server.go

@@ -575,7 +581,7 @@ func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *q
 	var panicked bool
 	func() {
 		defer func() {
-			if p := recover(); p != nil {
+			if p := recover(); p != nil && err != ErrAbortHandler {


Why not just reuse the http.ErrAbortHandler?

I think we have to be a little bit more careful here, the if here also sets panicked to true. We only want to prevent the logging, right?

I didn't know about http.ErrAbortHandler, let me look into that a bit more.

Isn't panicked only set if p is not nil and not ErrAbortHandler/http.ErrAbortHandler?

/me is an idiot and realizes he should have just used the variable from the stdlib instead of redefining it. Thanks, I'll do that.

Isn't panicked only set if p is not nil and not ErrAbortHandler/http.ErrAbortHandler?

What's the expectation when ErrAbortHandler is raised? I thought that the status code should be 500 then, not 200 (that's what panicked controls).

Oh, that's a good point. I didn't see the panicked controlled the return status a few lines below. hmm, is there a specific reason panicked is set last in that function that I'm missing? Could we just move it to the first thing to do in that function, and then if p is http.ErrAbortHandler return out of that function?

marten-seemann

@shade34321 Code LGTM now. Could you add a test case please?

Please also make sure to sign the CLA (see #3575 (comment)).

shade34321 · 2022-10-08T11:44:40Z

Yeah, I've been looking at server_test.go since yesterday trying to understand it a bit more before adding.

shade34321 · 2022-10-08T12:31:04Z

The tests passing locally for me. I'm a bit unsure if I did the tests right, I feel like I'm missing something.

quic-go/http3 on  http3_handle_abort via 🐹 v1.18 took 9s 
❯ go test
Running Suite: HTTP/3 Suite
===========================
Random Seed: 1665232217
Will run 173 of 173 specs

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Ran 173 of 173 Specs in 2.311 seconds
SUCCESS! -- 173 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS
ok      github.com/lucas-clemente/quic-go/http3 5.214s

quic-go/http3 on  http3_handle_abort via 🐹 v1.18 took 6s 
❯

shade34321 · 2022-10-08T12:31:59Z

@shade34321 Code LGTM now. Could you add a test case please?

Please also make sure to sign the CLA (see #3575 (comment)).

I had to amend my commits, I signed the CLA with the email associated with this account but forgot to set that correctly in my local git repo. It should be fixed now.

shade34321 · 2022-10-08T12:32:47Z

http3/server_test.go

@@ -201,6 +201,36 @@ var _ = Describe("Server", func() {
 			Expect(hfs).To(HaveKeyWithValue(":status", []string{"200"}))
 		})

+		It("handles aborts", func() {


This was just a simple test that I'm not 100% sure is testing what is needed to be tested. But it does test part IMHO.

The test below this should be sufficient.

Cool, thanks!

Can you remove this one?

shade34321 · 2022-10-08T12:33:59Z

http3/server_test.go

+		It("handles a aborting handler", func() {
+			s.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				panic(http.ErrAbortHandler)
+			})


This is supposed to be the more complete test, I wasn't sure how to mock an http abort and this is all I could think of. Is there something better?

shade34321 · 2022-10-09T00:05:07Z

I think all that is left for me is the circleci stuff, I'll try and take a look some tomorrow though I don't know if it's failing because of my change. When I ran the tests earlier off master it also failed.

Summarizing 1 Failure:

[Fail] Datagram test with QUIC version v1 [It] sends datagrams 
/home/circleci/project/integrationtests/self/datagram_test.go:67

marten-seemann · 2022-10-09T05:27:05Z

Don't worry about the datagram test, it's been flaky for a while.

codecov · 2022-10-09T11:16:20Z

Codecov Report

Base: 85.50% // Head: 85.51% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (0dd041a) compared to base (424a663).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3575   +/-   ##
=======================================
  Coverage   85.50%   85.51%           
=======================================
  Files         139      139           
  Lines       10244    10246    +2     
=======================================
+ Hits         8759     8761    +2     
  Misses       1103     1103           
  Partials      382      382

Impacted Files	Coverage Δ
http3/server.go	`74.45% <100.00%> (+0.16%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

shade34321 · 2022-10-09T11:51:23Z

Thanks for all of the help! I mentioned in the issue that I was working on this for hacktoberfest. Is it possible to have this project opt in so I can receive credit for it please. Thanks!

marten-seemann · 2022-10-09T13:58:26Z

I mentioned in the issue that I was working on this for hacktoberfest. Is it possible to have this project opt in so I can receive credit for it please.

I don't have the permissions on this repo. @lucas-clemente, can you help us here?

shade34321 · 2022-10-10T12:08:27Z

I think all that is needed is adding the label to the PR hacktoberfest-accepted but I understand if you don't have permission for that. Thanks anyways!

marten-seemann · 2022-10-11T07:23:10Z

I think all that is needed is adding the label to the PR hacktoberfest-accepted but I understand if you don't have permission for that.

That I can do.

mholt · 2022-10-13T17:42:17Z

Thank you for the fix, Marten!

marten-seemann reviewed Oct 7, 2022

View reviewed changes

marten-seemann reviewed Oct 8, 2022

View reviewed changes

marten-seemann changed the title ~~Handle abort like the stdlib does~~ http3: handle ErrAbortHandler when the handler panics Oct 8, 2022

shade34321 added 4 commits October 8, 2022 08:26

Handle abort like the stdlib does

5eb236b

Using the sentinel value from the stdlib instead of redefining.

d148d8c

we return before logging out

15031b1

Added test to hand abort handler

c5501cf

shade34321 force-pushed the http3_handle_abort branch from 2932240 to c5501cf Compare October 8, 2022 12:27

Added in two tests but apparently only saved the first one.

a5091d0

shade34321 commented Oct 8, 2022

View reviewed changes

marten-seemann added this to the v0.30.0 milestone Oct 9, 2022

remove one test case because it wasn't needed

0dd041a

marten-seemann approved these changes Oct 9, 2022

View reviewed changes

marten-seemann merged commit a905648 into quic-go:master Oct 9, 2022

marten-seemann mentioned this pull request Oct 9, 2022

abort directive returns http/500 on http/3 caddyserver/caddy#5125

Closed

marten-seemann added the hacktoberfest-accepted label Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http3: handle ErrAbortHandler when the handler panics #3575

http3: handle ErrAbortHandler when the handler panics #3575

shade34321 commented Oct 7, 2022

google-cla bot commented Oct 7, 2022

marten-seemann Oct 7, 2022

shade34321 Oct 7, 2022

shade34321 Oct 7, 2022

marten-seemann Oct 7, 2022

shade34321 Oct 7, 2022

marten-seemann left a comment

shade34321 commented Oct 8, 2022

shade34321 commented Oct 8, 2022

shade34321 commented Oct 8, 2022

shade34321 Oct 8, 2022

marten-seemann Oct 8, 2022

shade34321 Oct 9, 2022

marten-seemann Oct 9, 2022

shade34321 Oct 9, 2022

shade34321 Oct 8, 2022

shade34321 commented Oct 9, 2022

marten-seemann commented Oct 9, 2022

codecov bot commented Oct 9, 2022 •

edited

shade34321 commented Oct 9, 2022

marten-seemann commented Oct 9, 2022

shade34321 commented Oct 10, 2022

marten-seemann commented Oct 11, 2022

mholt commented Oct 13, 2022

http3: handle ErrAbortHandler when the handler panics #3575

http3: handle ErrAbortHandler when the handler panics #3575

Conversation

shade34321 commented Oct 7, 2022

google-cla bot commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann left a comment

Choose a reason for hiding this comment

shade34321 commented Oct 8, 2022

shade34321 commented Oct 8, 2022

shade34321 commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shade34321 commented Oct 9, 2022

marten-seemann commented Oct 9, 2022

codecov bot commented Oct 9, 2022 • edited

Codecov Report

shade34321 commented Oct 9, 2022

marten-seemann commented Oct 9, 2022

shade34321 commented Oct 10, 2022

marten-seemann commented Oct 11, 2022

mholt commented Oct 13, 2022

codecov bot commented Oct 9, 2022 •

edited