Skip to content

Conversation

@wmin0
Copy link

@wmin0 wmin0 commented Mar 15, 2018

Somehow server side would close connection actively (timeout, server close, etc...). Therefore, client should ack close in this situation, otherwise client process will be fulled of CLOSE_WAIT and stuck finally.

@wmin0 wmin0 force-pushed the wmin0-fix-connection-leak branch 2 times, most recently from d6b8906 to d6e1f31 Compare March 15, 2018 04:15
@wmin0
Copy link
Author

wmin0 commented Mar 15, 2018

https://golang.org/src/database/sql/sql.go#L1045 only close errBadConn. If lib/pq doesn't close connection when set conn.bad flag, the close of the bad connection will not be called.

@domac
Copy link

domac commented Dec 26, 2018

https://golang.org/src/database/sql/sql.go#L1045 only close errBadConn. If lib/pq doesn't close connection when set conn.bad flag, the close of the bad connection will not be called.

I would like to ask if the repair of this code works normally and can reduce the establishment of close_wait stuck?

@wmin0
Copy link
Author

wmin0 commented Dec 26, 2018

This pr I worked with was at golang 10, you can find golang source code I referred here https://github.com/golang/go/blob/release-branch.go1.10/src/database/sql/sql.go#L1045.
And @domac your question is yes, or I don't propose this pr ._.
Maybe I should add some stress test in bench_test.go.

@domac
Copy link

domac commented Dec 26, 2018

This pr I worked with was at golang 10, you can find golang source code I referred here https://github.com/golang/go/blob/release-branch.go1.10/src/database/sql/sql.go#L1045.
And @domac your question is yes, or I don't propose this pr ._.
Maybe I should add some stress test in bench_test.go.

Thank you your reply.... This means that if my go version is 1.9, I need to modify it in the way of your previous pr (d6b8906) to avoid the close_wait problem?

@wmin0
Copy link
Author

wmin0 commented Dec 26, 2018

https://github.com/golang/go/blob/release-branch.go1.9/src/database/sql/sql.go#L946
In golang 1.9, the leak seems to be worse because of it only closes connection on timeout. This pr should work on it.

@wmin0 wmin0 force-pushed the wmin0-fix-connection-leak branch from d6e1f31 to f51b554 Compare December 26, 2018 06:53
@wmin0
Copy link
Author

wmin0 commented Dec 26, 2018

My ci still fails because of race condition but it's not related to this pr ....
https://travis-ci.org/lib/pq/jobs/472262943

@arp242 arp242 added bug needs-test Needs a test before it can be merged labels Jan 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug needs-test Needs a test before it can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants