Fix panic stack trace being printed during recovery of broken pipe (#1089) #1259

justinfx · 2018-02-27T03:19:47Z

This is a proposed fix for #1089 to prevent the case of a broken pipe leading to a noisy panic stack trace.
The DumpRequest output is still preserved, but the panic is avoided, which is caused by the RecoveryWithWriter handler trying to write headers after the body has already been written.

…in-gonic#1089)

codecov · 2018-02-27T03:31:56Z

Codecov Report

Merging #1259 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1259      +/-   ##
==========================================
+ Coverage   99.28%   99.28%   +<.01%     
==========================================
  Files          40       40              
  Lines        1958     1971      +13     
==========================================
+ Hits         1944     1957      +13     
  Misses         10       10              
  Partials        4        4

Impacted Files	Coverage Δ
recovery.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b47a8...9cbf651. Read the comment docs.

justinfx · 2018-02-27T10:00:25Z

I can add test coverage for this change, tomorrow, if needed for acceptance.

justinfx · 2018-02-28T02:38:08Z

Ok, this is failing on travis because tests currently require go-1.6 to pass, and I am using sub-tests. Is this an outdated requirement? Does code really still need to conform to go-1.6 and do I need to replace the sub-tests with manual table tests?

justinfx · 2018-03-08T20:54:02Z

:bump:?

justinfx · 2018-09-14T20:16:23Z

Anyone care about this fix being merged?

thinkerou · 2018-09-16T13:25:51Z

@justinfx Hi, please fix conflict, thanks!

justinfx · 2018-09-17T04:56:35Z

@thinkerou I have fixed the conflicts, but I unless I remove the go-1.6.x line from the travis config, it will still not be able to use sub-tests. Can you confirm if you need to keep testing against go 1.6 or can I remove that requirement in this merge?

…gin-gonic#1259)

Huang-lin · 2018-10-11T06:27:54Z

What is the current situation of this pr, will anyone continue to handle it?

justinfx · 2018-10-11T07:03:00Z

If all that is holding this up is that I am not allowed to use >go-1.6 features, I guess I can conform it.

justinfx · 2018-10-30T00:35:58Z

:bump: maintainers?

thinkerou · 2018-10-30T01:10:57Z

@justinfx have some conflict and please fix it, thanks!

justinfx · 2018-10-30T02:01:41Z

@thinkerou , I am finding this process very frustrating and demotivating. I've been asking for this to be merged for 8 months. When it got out of sync with master, I fixed it again. I've been asking multiple times if the travis configuration can please be updated to test > go 1.6 so that I don't have to remove the use of subtests, but there has been no answer to that. Now that it has been sitting again for a while, I am being asked to fixed the conflicts.

How many times do I need to go through this loop before it gets merged? I really want to see this fixed, but I need to know that there is a point to this process.

thinkerou · 2018-11-03T10:37:29Z

@justinfx so sorry about your said, we also use spare time (late at night or early morning) to review code, because we need to work, hope that you can understand, very very thanks!

Because go1.6 not support testing.Run, I think you should use // +build go1.7, now gin can't remove go1.6 from travis ci. we plan gin1.4 or gin1.5 remove it.

* chore: add a version file includes gin version * update version for dev version

…nic#1561) * chore: recover go master build, partial revert gin-gonic#1514 * chore: add master to go branch build targets

…in-gonic#1529)

Fixes gin-gonic#1331 HTTP logging leaks sensitive request information. This PR removes HTTP request logging during panics.

* support go module * update golint package url * update golint

* init docs dir * add middleware document * fix indent * update docs

ref gin-gonic#1569 (comment)

Digging into the test code base I've found out that some of the tests for `LoadHTML*` methods are not reliable and efficient. They use timeouts to be sure that goroutine with the server has started. And even more, in old implementation, the server started only once – all the new instances silently failed due to the occupied network port. Here is a short overview of the proposed changes: - it's not necessary to rely on timeouts, the server starts listening synchronously and returns control when it is ready - once the server is run, it's stopped after a test passes - dry out http server setup - magic with empty closure return is eliminated - preserve router.RunTLS coverage with integration tests

When `gin.Context.FormFile("...")` is called the `engine.MaxMultipartMemory` is never used. This PR makes sure that the `MaxMultipartMemory` is passed and removes 2 calls to `http.Request.ParseForm` since they are called from `http.Request.ParseMultipartForm`

FIX r.LoadHTMLGlob("/path/to/templates")) to r.LoadHTMLGlob("/path/to/templates")

The following comments to vars, conts and method were added to pass `golinter` with 100%. ![captura de pantalla 2018-10-31 a la s 15 23 37](https://user-images.githubusercontent.com/10160626/47819725-faba3780-dd20-11e8-978c-1b3ab7de26ed.png)

In XHTML, the <input> tag must be properly closed, like this `<input />`. In HTML5 the `<input>` tag has no ending slash. https://www.w3schools.com/tags/tag_input.asp

The `<link>` element is an empty element, it contains attributes only. In HTML5 the `<link>` tag has no end tag. In XHTML the `<link>` tag must be properly closed.

Missing the right colon

justinfx · 2018-11-03T23:47:36Z

Thanks for the build tag suggestion. I have rebased to master and added the tag to make everything pass.
I completely appreciate the time it takes to maintain open source projects in our free time. I have a couple that I also maintain. But if someone submits a working patch, I do try to get it merged soonish since they put in the work. It also takes 2 or 3 more sessions for the submitter if they have to keep rebasing to keep it in a mergeable state for the maintainers. But it is clean now so hopefully it will be merged!
Thanks again.

thinkerou · 2018-11-05T05:05:30Z

@justinfx thanks, need @appleboy help review, thanks!

justinfx · 2018-11-05T09:57:35Z

🍾

Fix panic stack trace being printed during recovery of broken pipe (g…

80916dc

…in-gonic#1089)

Justin Israel and others added 2 commits February 28, 2018 15:11

Add test for recovery of broken pipe (gin-gonic#1089)

133a62d

Fix spelling in test for recovery of broken pipe (gin-gonic#1089)

d3bcb45

Merge branch 'master' into 1089_broken_pipe

e7078e9

justinfx added 2 commits September 17, 2018 17:04

Fix stack function not being called with params (merge conflict) (refs …

97c4c98

…gin-gonic#1259)

gofmt recovery changes (refs gin-gonic#1259)

d3a05e4

thinkerou requested review from appleboy, javierprovecho and thinkerou September 17, 2018 10:47

thinkerou and others added 8 commits November 4, 2018 11:20

chore: add a version file includes gin version (gin-gonic#1549)

076e182

* chore: add a version file includes gin version * update version for dev version

feat: replace debug log with fmt package. (gin-gonic#1560)

10ba502

chore: recover go master build, partial revert gin-gonic#1514 (gin-go…

8a7ccc3

…nic#1561) * chore: recover go master build, partial revert gin-gonic#1514 * chore: add master to go branch build targets

chore: fix typo and add a little anotation (gin-gonic#1562)

500ad23

improve panic information when a catch-all wildcard conflict occurs (g…

d02ae1e

…in-gonic#1529)

add release badge for readme (gin-gonic#1533)

1e326a1

Don't log requests (gin-gonic#1370)

5548cc3

Fixes gin-gonic#1331 HTTP logging leaks sensitive request information. This PR removes HTTP request logging during panics.

ci: fast finish when build failed (gin-gonic#1568)

946eba9

thinkerou and others added 15 commits November 4, 2018 11:56

attempt to support go module (gin-gonic#1569)

e3a3b89

* support go module * update golint package url * update golint

document: add docs dir and middleware document (gin-gonic#1521)

d369746

* init docs dir * add middleware document * fix indent * update docs

vendor: upgrade some dependency package version (gin-gonic#1596)

37c19a7

ref gin-gonic#1569 (comment)

add example to set and get cookies (gin-gonic#1599)

de4743e

add gin user - photoprism (gin-gonic#1601)

64ea6f1

Expose HandlerFunc in RouteInfos (gin-gonic#1272)

c03447e

FIX r.LoadHTMLGlob("/path/to/templates") (gin-gonic#1616)

4940983

FIX r.LoadHTMLGlob("/path/to/templates")) to r.LoadHTMLGlob("/path/to/templates")

Change HTML input tags to use HTML5 syntax. (gin-gonic#1617)

6c0566e

In XHTML, the <input> tag must be properly closed, like this `<input />`. In HTML5 the `<input>` tag has no ending slash. https://www.w3schools.com/tags/tag_input.asp

Change HTML link tags to use HTML5 syntax. (gin-gonic#1621)

7f6ddf2

The `<link>` element is an empty element, it contains attributes only. In HTML5 the `<link>` tag has no end tag. In XHTML the `<link>` tag must be properly closed.

Update README.md (gin-gonic#1620)

17b319b

Missing the right colon

Only build recovery tests for go >=1.7

a3804b6

Merge branch 'master' into 1089_broken_pipe

92abfec

thinkerou approved these changes Nov 5, 2018

View reviewed changes

appleboy added bug enhancement labels Nov 5, 2018

appleboy added this to the 1.4 milestone Nov 5, 2018

appleboy approved these changes Nov 5, 2018

View reviewed changes

Merge branch 'master' into 1089_broken_pipe

9cbf651

thinkerou merged commit 37854ee into gin-gonic:master Nov 6, 2018

thinkerou mentioned this pull request Nov 6, 2018

write: broken pipe in func (r Data) Render #1089

Closed

justinfx deleted the 1089_broken_pipe branch June 20, 2019 23:26

This was referenced Dec 14, 2020

Bug/"goroutine crowdsec/controllersV1/StreamDecision crashed" crowdsecurity/crowdsec#532

Closed

gin: broken pipe crowdsecurity/crowdsec#538

Merged

Tevic mentioned this pull request Jan 26, 2022

return err instead of panic in Render #3039

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic stack trace being printed during recovery of broken pipe (#1089) #1259

Fix panic stack trace being printed during recovery of broken pipe (#1089) #1259

justinfx commented Feb 27, 2018 •

edited

codecov bot commented Feb 27, 2018 •

edited

justinfx commented Feb 27, 2018

justinfx commented Feb 28, 2018 •

edited

justinfx commented Mar 8, 2018

justinfx commented Sep 14, 2018

thinkerou commented Sep 16, 2018

justinfx commented Sep 17, 2018

Huang-lin commented Oct 11, 2018

justinfx commented Oct 11, 2018

justinfx commented Oct 30, 2018

thinkerou commented Oct 30, 2018

justinfx commented Oct 30, 2018

thinkerou commented Nov 3, 2018

justinfx commented Nov 3, 2018

thinkerou commented Nov 5, 2018

justinfx commented Nov 5, 2018

Fix panic stack trace being printed during recovery of broken pipe (#1089) #1259

Fix panic stack trace being printed during recovery of broken pipe (#1089) #1259

Conversation

justinfx commented Feb 27, 2018 • edited

codecov bot commented Feb 27, 2018 • edited

Codecov Report

justinfx commented Feb 27, 2018

justinfx commented Feb 28, 2018 • edited

justinfx commented Mar 8, 2018

justinfx commented Sep 14, 2018

thinkerou commented Sep 16, 2018

justinfx commented Sep 17, 2018

Huang-lin commented Oct 11, 2018

justinfx commented Oct 11, 2018

justinfx commented Oct 30, 2018

thinkerou commented Oct 30, 2018

justinfx commented Oct 30, 2018

thinkerou commented Nov 3, 2018

justinfx commented Nov 3, 2018

thinkerou commented Nov 5, 2018

justinfx commented Nov 5, 2018

justinfx commented Feb 27, 2018 •

edited

codecov bot commented Feb 27, 2018 •

edited

justinfx commented Feb 28, 2018 •

edited