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

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

Merged
merged 40 commits into from
Nov 6, 2018

Conversation

justinfx
Copy link
Contributor

@justinfx justinfx commented Feb 27, 2018

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.

@codecov
Copy link

codecov bot commented Feb 27, 2018

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

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

@justinfx
Copy link
Contributor Author

justinfx commented Feb 28, 2018

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
Copy link
Contributor Author

justinfx commented Mar 8, 2018

:bump:?

@justinfx
Copy link
Contributor Author

Anyone care about this fix being merged?

@thinkerou
Copy link
Member

@justinfx Hi, please fix conflict, thanks!

@justinfx
Copy link
Contributor Author

@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?

@Huang-lin
Copy link

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

@justinfx
Copy link
Contributor Author

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
Copy link
Contributor Author

:bump: maintainers?

@thinkerou
Copy link
Member

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

@justinfx
Copy link
Contributor Author

@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
Copy link
Member

@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.

thinkerou and others added 15 commits November 4, 2018 11:56
* support go module

* update golint package url

* update golint
* init docs dir

* add middleware document

* fix indent

* update docs
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
Copy link
Contributor Author

justinfx commented Nov 3, 2018

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
Copy link
Member

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

@appleboy appleboy added this to the 1.4 milestone Nov 5, 2018
@justinfx
Copy link
Contributor Author

justinfx commented Nov 5, 2018

🍾

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

Successfully merging this pull request may close these issues.