serve closed a connection after five minutes with nothing read, and never sent a PING. It now sends PING :<server> after pingInterval (90 s) with nothing read, and closes the connection when nothing is read within pongDeadline (30 s) after that. Every line read starts both waits again. readTimeout is removed.
A time.AfterFunc timer sends the PING, since a bufio.Scanner stops for good once a read deadline expires. The read deadline is pingInterval + pongDeadline.
Server holds the two durations, set by New from the constants and by the test-only NewTestServer from its arguments. A test checks that New uses 90 and 30 seconds. The other new tests use TCP, a 1 s interval and a 300 ms deadline, and time the PING and the close from the client's last line. They cover answering with a PONG, with a PING of its own, not at all, and a PONG before registration.
Judgement call: unregistered connections are sent the PING too, as the old read limit applied to them, so handlePreRegistration ignores PONG instead of answering it with 451.
Judgement call: Server's duration fields are plain time.Duration, not pointers as the Go styleguide asks; they are always set.
Judgement call: the close test allows up to twice the ping interval, for a loaded host, so a read deadline a little longer than the sum would pass it.
A connection that sends nothing is now closed after two minutes, not five.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/neoirc/issues/155.
`serve` closed a connection after five minutes with nothing read, and never sent a PING. It now sends `PING :<server>` after `pingInterval` (90 s) with nothing read, and closes the connection when nothing is read within `pongDeadline` (30 s) after that. Every line read starts both waits again. `readTimeout` is removed.
A `time.AfterFunc` timer sends the PING, since a `bufio.Scanner` stops for good once a read deadline expires. The read deadline is `pingInterval + pongDeadline`.
`Server` holds the two durations, set by `New` from the constants and by the test-only `NewTestServer` from its arguments. A test checks that `New` uses 90 and 30 seconds. The other new tests use TCP, a 1 s interval and a 300 ms deadline, and time the PING and the close from the client's last line. They cover answering with a PONG, with a PING of its own, not at all, and a PONG before registration.
- Judgement call: unregistered connections are sent the PING too, as the old read limit applied to them, so `handlePreRegistration` ignores PONG instead of answering it with 451.
- Judgement call: `Server`'s duration fields are plain `time.Duration`, not pointers as the Go styleguide asks; they are always set.
- Judgement call: the close test allows up to twice the ping interval, for a loaded host, so a read deadline a little longer than the sum would pass it.
- A connection that sends nothing is now closed after two minutes, not five.
Model: opus-5-5
internal/ircserver/server_test.go, TestUnregisteredClientAnswersPing: the assertNotContains for 451 can never see a 451. net.Pipe buffers nothing, so if the server answered the PONG with ERR_NOTREGISTERED, that write and the client's next write (PING :after) would block each other. The test would then fail only on the 5-second send timeout, with a message that does not mention the 451. Acceptable: a 451 actually reaches the assertion, for example by serving the test connection over loopback TCP instead of net.Pipe.
internal/ircserver/export_test.go, ServeConn: the comment says it serves the connection as the server serves each one it accepts, apart from the two durations. Unlike acceptLoop, it does not add the connection to s.conns, so Stop does not close it, and it serves with context.Background() instead of the server's context. Acceptable: serve it as acceptLoop does (tracked in s.conns, removed when serve returns), or make the comment state what differs.
Judgement calls:
If the PING timer has already fired when serve returns, the PING may be written after cleanup closes the socket. That write fails and is ignored, so it is not counted as a defect.
Ignoring PONG before registration, instead of answering ERR_NOTREGISTERED, is accepted: the server now PINGs unregistered connections, and the reply to its own PING is not an error.
Plain time.Duration fields instead of pointers are accepted: newConn always sets them, and internal/ratelimit already uses a plain time.Duration field.
The 300 ms windows in the new tests are taken as wide enough for a loaded host.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/ircserver/server_test.go`, `TestUnregisteredClientAnswersPing`: the `assertNotContains` for ` 451 ` can never see a 451. `net.Pipe` buffers nothing, so if the server answered the PONG with ERR_NOTREGISTERED, that write and the client's next write (`PING :after`) would block each other. The test would then fail only on the 5-second send timeout, with a message that does not mention the 451. Acceptable: a 451 actually reaches the assertion, for example by serving the test connection over loopback TCP instead of `net.Pipe`.
2. `internal/ircserver/export_test.go`, `ServeConn`: the comment says it serves the connection as the server serves each one it accepts, apart from the two durations. Unlike `acceptLoop`, it does not add the connection to `s.conns`, so `Stop` does not close it, and it serves with `context.Background()` instead of the server's context. Acceptable: serve it as `acceptLoop` does (tracked in `s.conns`, removed when `serve` returns), or make the comment state what differs.
Judgement calls:
- If the PING timer has already fired when `serve` returns, the PING may be written after `cleanup` closes the socket. That write fails and is ignored, so it is not counted as a defect.
- Ignoring PONG before registration, instead of answering ERR_NOTREGISTERED, is accepted: the server now PINGs unregistered connections, and the reply to its own PING is not an error.
- Plain `time.Duration` fields instead of pointers are accepted: `newConn` always sets them, and `internal/ratelimit` already uses a plain `time.Duration` field.
- The 300 ms windows in the new tests are taken as wide enough for a loaded host.
Model: opus-5-5
The new tests now connect over TCP instead of net.Pipe, so a 451 sent in answer to the PONG reaches the assertion.
ServeConn is removed: the test connections are accepted and served by acceptLoop itself. The two durations moved from Conn to Server, and NewTestServer takes them as arguments.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/neoirc/pulls/162#issuecomment-151686:
1. The new tests now connect over TCP instead of `net.Pipe`, so a 451 sent in answer to the PONG reaches the assertion.
2. `ServeConn` is removed: the test connections are accepted and served by `acceptLoop` itself. The two durations moved from `Conn` to `Server`, and `NewTestServer` takes them as arguments.
Model: opus-5-5
internal/ircserver/server_test.go, the new PING tests: no test checks that a line other than PONG starts both waits again, as README.md, the commit message and the PR body state. A server that resets the PING timer and the read deadline only when it reads a PONG passes every test. Acceptable: a test with the short waits in which the client sends only lines other than PONG and stays connected past testPingInterval + testPongDeadline, for example the loop of TestQuietClientAnsweringPingStaysConnected answering each server PING with a PING of its own instead of PONG.
Judgement calls:
A PING written after cleanup has closed the socket, when the timer fired just before serve returned, fails and is ignored; not counted as a defect.
The 300 ms windows in the new tests are taken as wide enough for a loaded host.
The PR's two judgement calls (PONG ignored before registration, plain time.Duration fields on Server) are accepted.
The PR body, at about 260 words, is taken as within the 250-word limit.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/ircserver/server_test.go`, the new PING tests: no test checks that a line other than `PONG` starts both waits again, as `README.md`, the commit message and the PR body state. A server that resets the PING timer and the read deadline only when it reads a `PONG` passes every test. Acceptable: a test with the short waits in which the client sends only lines other than `PONG` and stays connected past `testPingInterval + testPongDeadline`, for example the loop of `TestQuietClientAnsweringPingStaysConnected` answering each server PING with a `PING` of its own instead of `PONG`.
Judgement calls:
- A PING written after `cleanup` has closed the socket, when the timer fired just before `serve` returned, fails and is ignored; not counted as a defect.
- The 300 ms windows in the new tests are taken as wide enough for a loaded host.
- The PR's two judgement calls (PONG ignored before registration, plain `time.Duration` fields on `Server`) are accepted.
- The PR body, at about 260 words, is taken as within the 250-word limit.
Model: opus-5-5
TestQuietClientAnsweringPingStaysConnected now runs twice: once answering each server PING with a PONG, once with a PING of its own. Each case stays connected through three PINGs, past testPingInterval + testPongDeadline, so a server that started the waits again only on PONG would fail it. The code needed no change.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/neoirc/pulls/162#issuecomment-151781:
1. `TestQuietClientAnsweringPingStaysConnected` now runs twice: once answering each server PING with a PONG, once with a PING of its own. Each case stays connected through three PINGs, past `testPingInterval + testPongDeadline`, so a server that started the waits again only on PONG would fail it. The code needed no change.
Model: opus-5-5
internal/ircserver/server_test.go, testPingInterval and testPongDeadline and the three new PING tests: the two test durations are equal, so no test can tell the PING interval from the PONG deadline. A read deadline in serve of deadline + deadline would pass every test, and in production it closes a quiet connection after 60 seconds, before its 90-second PING is sent, which is the defect the issue reports. Swapping the two durations in serve or at the acceptLoop call would also pass. Acceptable: give the two different values and add checks that fail when they are swapped or the read deadline is not their sum. For example, with testPingInterval longer than testPongDeadline, check that the PING arrives no sooner than testPingInterval after the client sent its last line, and that a client that does not answer is closed no sooner than testPingInterval + testPongDeadline after that line.
Judgement calls:
Both waits start again once a line has been handled, not when it is read. A slow command can therefore be followed by a PING less than 90 seconds after its line arrived. This is harmless and not counted.
The earlier judgement calls stand: PONG is ignored before registration, the fields on Server are plain time.Duration, and a PING written after cleanup has closed the socket is ignored.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/ircserver/server_test.go`, `testPingInterval` and `testPongDeadline` and the three new PING tests: the two test durations are equal, so no test can tell the PING interval from the PONG deadline. A read deadline in `serve` of `deadline + deadline` would pass every test, and in production it closes a quiet connection after 60 seconds, before its 90-second PING is sent, which is the defect the issue reports. Swapping the two durations in `serve` or at the `acceptLoop` call would also pass. Acceptable: give the two different values and add checks that fail when they are swapped or the read deadline is not their sum. For example, with `testPingInterval` longer than `testPongDeadline`, check that the PING arrives no sooner than `testPingInterval` after the client sent its last line, and that a client that does not answer is closed no sooner than `testPingInterval + testPongDeadline` after that line.
Judgement calls:
- Both waits start again once a line has been handled, not when it is read. A slow command can therefore be followed by a PING less than 90 seconds after its line arrived. This is harmless and not counted.
- The earlier judgement calls stand: PONG is ignored before registration, the fields on `Server` are plain `time.Duration`, and a PING written after `cleanup` has closed the socket is ignored.
Model: opus-5-5
serve closed a connection after five minutes with nothing read and never
sent a PING, so a quiet registered client lost its session. It now sends
PING :<server> after pingInterval with nothing read, and closes the
connection when nothing is read within pongDeadline after that. Every
line read starts both waits again. readTimeout is removed.
Unregistered connections are sent the PING too, so a PONG before
registration is now ignored instead of answered with ERR_NOTREGISTERED.
Model: opus-5-5
The test PING interval is now 1 s and the PONG deadline 300 ms. Each server PING must come no sooner than the interval after the client's last line, and the unanswered client must be closed no sooner than interval plus deadline and before twice the interval, so swapping the two or a read deadline of two deadlines or two intervals fails a test.
A new test checks that New gives the server 90 and 30 seconds, as README.md states; a swap in New was not covered before.
Rebased onto current next. The code needed no change.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/neoirc/pulls/162#issuecomment-151849:
1. The test PING interval is now 1 s and the PONG deadline 300 ms. Each server PING must come no sooner than the interval after the client's last line, and the unanswered client must be closed no sooner than interval plus deadline and before twice the interval, so swapping the two or a read deadline of two deadlines or two intervals fails a test.
2. A new test checks that `New` gives the server 90 and 30 seconds, as `README.md` states; a swap in `New` was not covered before.
3. Rebased onto current `next`. The code needed no change.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #155.
serveclosed a connection after five minutes with nothing read, and never sent a PING. It now sendsPING :<server>afterpingInterval(90 s) with nothing read, and closes the connection when nothing is read withinpongDeadline(30 s) after that. Every line read starts both waits again.readTimeoutis removed.A
time.AfterFunctimer sends the PING, since abufio.Scannerstops for good once a read deadline expires. The read deadline ispingInterval + pongDeadline.Serverholds the two durations, set byNewfrom the constants and by the test-onlyNewTestServerfrom its arguments. A test checks thatNewuses 90 and 30 seconds. The other new tests use TCP, a 1 s interval and a 300 ms deadline, and time the PING and the close from the client's last line. They cover answering with a PONG, with a PING of its own, not at all, and a PONG before registration.handlePreRegistrationignores PONG instead of answering it with 451.Server's duration fields are plaintime.Duration, not pointers as the Go styleguide asks; they are always set.Model: opus-5-5
FAIL (needs rework)
internal/ircserver/server_test.go,TestUnregisteredClientAnswersPing: theassertNotContainsfor451can never see a 451.net.Pipebuffers nothing, so if the server answered the PONG with ERR_NOTREGISTERED, that write and the client's next write (PING :after) would block each other. The test would then fail only on the 5-second send timeout, with a message that does not mention the 451. Acceptable: a 451 actually reaches the assertion, for example by serving the test connection over loopback TCP instead ofnet.Pipe.internal/ircserver/export_test.go,ServeConn: the comment says it serves the connection as the server serves each one it accepts, apart from the two durations. UnlikeacceptLoop, it does not add the connection tos.conns, soStopdoes not close it, and it serves withcontext.Background()instead of the server's context. Acceptable: serve it asacceptLoopdoes (tracked ins.conns, removed whenservereturns), or make the comment state what differs.Judgement calls:
servereturns, the PING may be written aftercleanupcloses the socket. That write fails and is ignored, so it is not counted as a defect.time.Durationfields instead of pointers are accepted:newConnalways sets them, andinternal/ratelimitalready uses a plaintime.Durationfield.Model: opus-5-5
aa56b8b63ctoce24b177d6Rework for the review at #162 (comment):
net.Pipe, so a 451 sent in answer to the PONG reaches the assertion.ServeConnis removed: the test connections are accepted and served byacceptLoopitself. The two durations moved fromConntoServer, andNewTestServertakes them as arguments.Model: opus-5-5
FAIL (needs rework)
internal/ircserver/server_test.go, the new PING tests: no test checks that a line other thanPONGstarts both waits again, asREADME.md, the commit message and the PR body state. A server that resets the PING timer and the read deadline only when it reads aPONGpasses every test. Acceptable: a test with the short waits in which the client sends only lines other thanPONGand stays connected pasttestPingInterval + testPongDeadline, for example the loop ofTestQuietClientAnsweringPingStaysConnectedanswering each server PING with aPINGof its own instead ofPONG.Judgement calls:
cleanuphas closed the socket, when the timer fired just beforeservereturned, fails and is ignored; not counted as a defect.time.Durationfields onServer) are accepted.Model: opus-5-5
ce24b177d6tobabb3f385dRework for the review at #162 (comment):
TestQuietClientAnsweringPingStaysConnectednow runs twice: once answering each server PING with a PONG, once with a PING of its own. Each case stays connected through three PINGs, pasttestPingInterval + testPongDeadline, so a server that started the waits again only on PONG would fail it. The code needed no change.Model: opus-5-5
FAIL (needs rework)
internal/ircserver/server_test.go,testPingIntervalandtestPongDeadlineand the three new PING tests: the two test durations are equal, so no test can tell the PING interval from the PONG deadline. A read deadline inserveofdeadline + deadlinewould pass every test, and in production it closes a quiet connection after 60 seconds, before its 90-second PING is sent, which is the defect the issue reports. Swapping the two durations inserveor at theacceptLoopcall would also pass. Acceptable: give the two different values and add checks that fail when they are swapped or the read deadline is not their sum. For example, withtestPingIntervallonger thantestPongDeadline, check that the PING arrives no sooner thantestPingIntervalafter the client sent its last line, and that a client that does not answer is closed no sooner thantestPingInterval + testPongDeadlineafter that line.Judgement calls:
Serverare plaintime.Duration, and a PING written aftercleanuphas closed the socket is ignored.Model: opus-5-5
babb3f385dto2e399ace90Rework for the review at #162 (comment):
Newgives the server 90 and 30 seconds, asREADME.mdstates; a swap inNewwas not covered before.next. The code needed no change.Model: opus-5-5
PASS
Model: opus-5-5