Check for dead sockets before timeout and enforce timeouts #806
Conversation
juanjux
force-pushed
the
deadsocket-check
branch
from
3456364
to
1c507ed
Compare
juanjux
changed the title
- Added test for row timeout - Review feedback and some simplified logic for the row read select - RowTimeout: Use a single NewTimer instead of After for better memory usage - Restore default timeout and readTimeout values - Refactor rowloop select Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
juanjux
force-pushed
the
deadsocket-check
branch
2 times, most recently
from
e806bcd
to
0ce415e
Compare
juanjux
force-pushed
the
deadsocket-check
branch
2 times, most recently
from
81f8664
to
141ab18
Compare
juanjux
force-pushed
the
deadsocket-check
branch
from
141ab18
to
1775a6f
Compare
…ux only) Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
juanjux
force-pushed
the
deadsocket-check
branch
from
1775a6f
to
7388b75
Compare
juanjux
changed the title
| func tcpSocks(accept AcceptFn) ([]sockTabEntry, error) { | ||
| f, err := os.Open(pathTCPTab) | ||
| defer func() { | ||
| _ = f.Close() |
There was a problem hiding this comment.
if you're ignoring the error of f.Close() then just defer f.Close()
There was a problem hiding this comment.
It's so GolangCI shut ups (and if he doesn't this doesn't pass the merge checks), which it doesn't which just a defer close()
There was a problem hiding this comment.
Approved since my suite passes.
According to tests that you wanted to add, here's my suite https://github.com/src-d/regression-gitbase/blob/a4afd0e8b11096bfc98fb722cc2f8d3cdec6ec37/cmd/regression-bblfsh-mockups/main.go#L52
But I think these cases are represented in your tests on the lower level.
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
|
Yesterday I tried many things to be able to get rid of the timeout-enforcing goroutine, without success. I'll try a couple things more today and change the status to mergeable anyway. Windows support will come in another PR so I don't stall this too much, since it's very needed. |
juanjux
changed the title
|
Ok, so tried keepalives (higher Go level and lower level using syscalls), deadlines, and many other things. Looks like we can't get rid of the second goroutine so since this works this could be merged when reviews pass. PTAL @erizocosmico @ajnavarro. I'll work on adding Windows support for the dead socket detector on another PR. |
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
juanjux
force-pushed
the
deadsocket-check
branch
from
5107b5f
to
6aa453e
Compare
|
@ajnavarro gofmt ran on the entire project and goimports on the modified directories. |
Fixes #800 at least in Linux (once merged I'll open issues for Darwin and Windows).
Related to src-d/gitbase-spark-connector-enterprise#81
This supersedes #801, the timeout checking part is basically the same with minor changes. What this adds is another goroutine that, using the Linux
/procfilesystem will check if the socket is closed and cancel the connection and queries in that case.While this is mergeable, I'll ask to wait until I say it can be merged because I want to try to get rid of the timeout checking goroutine. The current problem is that currently even if there is a timeout, the socket state won't change to
CLOSE_WAITuntil there is a realread()orwrite()to it so the timeout checking goroutine is still needed, but I wan't to try to configure keepalives for the connection and see if with this the timeout actually changes the socket state (and thus the connection checking goroutine can reap it). Also I would like to get an Ok from @lwsanty that found a problem in the previous PR.Bonus: a couple typos fixed in the tests.
cc @lwsanty : please check that this branch works for you with your previously failing test (and if you can link the test so I can maybe add it to the unittests of this project).