Check how servers handle interrupt

CHANGELOG.md

@@ -28,3 +28,5 @@ Initial release.
 - [#10](https://github.com/nrepl/proof/pull/10): Binaries for Linux, macOS
   and Windows on every release, and a GitHub Action for running proof in the
   CI of a server.
+- [#11](https://github.com/nrepl/proof/pull/11): Checks for `interrupt`, and
+  scenarios for nREPL's interrupt bugs in `proof serve`.

PLAN.md

@@ -64,27 +64,31 @@ lookups and completions, plus requests that go wrong.
 
 ### interrupt
 
-- Checks for interrupting a running eval (the eval gets `interrupted`
-  and `done`, the interrupt gets a `done` of its own), interrupting an
-  idle session (`session-idle`), an `interrupt-id` that doesn't match
-  and the session working afterwards.
-- An interrupt sent right after its eval. nREPL marks a session as
-  running only once the eval leaves the session's queue, so such an
-  interrupt can find the session idle
-  ([nrepl#504](https://github.com/nrepl/nrepl/issues/504)). The evals
-  waiting behind an interrupted one
-  ([nrepl#503](https://github.com/nrepl/nrepl/issues/503)) and the
-  exception of an interrupted eval, which nREPL sends after its `done`
-  ([nrepl#506](https://github.com/nrepl/nrepl/issues/506)), need checks
-  too.
-- Where an interrupted eval's `done` goes when the interrupt comes from
-  another connection
-  ([nrepl#498](https://github.com/nrepl/nrepl/pull/498)). Once an nREPL
-  release has the fix, `profiles/clojure.toml` and `proof serve` move to
-  it.
-- A step option for client profiles that sends a request without waiting
-  for its reply, so profiles can send `interrupt` and `stdin` while an
-  eval is running, the way CIDER, Calva and Conjure do.
+- Done: checks for interrupting a running eval, with and without
+  `interrupt-id` (Conjure never sends one), the `interrupted` status, an
+  idle session, an `interrupt-id` that doesn't match and the session
+  working afterwards.
+- Done: the evals waiting behind an interrupted one, which nREPL 1.8.0
+  and Babashka drop
+  ([nrepl#503](https://github.com/nrepl/nrepl/issues/503)), and where an
+  interrupted eval's `done` goes when the interrupt comes from another
+  connection, which nREPL's main branch gets right now
+  ([nrepl#498](https://github.com/nrepl/nrepl/pull/498)). The exception
+  of an interrupted eval, which nREPL sends after its `done`
+  ([nrepl#506](https://github.com/nrepl/nrepl/issues/506)), is
+  `wire.after-done`'s warning.
+- Done: `proof serve` interrupts like dialtone and repartee do, and has
+  nREPL's bugs as scenarios (`late-exception`, `drop-queued` and
+  `interrupt-elsewhere`), which `-like clojure` and `-like babashka`
+  turn on.
+- Not done: an interrupt sent right after its eval, which can find
+  nREPL's session idle
+  ([nrepl#504](https://github.com/nrepl/nrepl/issues/504)). That's a
+  race, so a check can't count on hitting it.
+- Not needed: a step option for client profiles that sends a request
+  without waiting for its reply. The `interrupt` requests of CIDER,
+  Calva and vim-fireplace are plain enough for `interrupt.running`, and
+  Conjure's for `interrupt.no-id`.
 
 ### load-file
 

README.md

@@ -131,14 +131,14 @@ the replies). `-like jank` turns on all of jank's scenarios at once, and
 ## Status
 
 proof is still in its early days. Right now it covers the core of the
-protocol (`describe`, unknown ops, `eval`, sessions, `stdin` and the wire format),
+protocol (`describe`, unknown ops, `eval`, sessions, `stdin`, `interrupt` and the wire format),
 what CIDER, Calva, Conjure, vim-fireplace, REPLy and neorepl send while
 connecting and evaluating code, the requests
 clients send and the server differences clients have to deal with, and
 `proof list` will show you all the checks.
 
-Checks for `interrupt`, `load-file`, lookups, completions and malformed
-requests are coming next, along with the first release.
+Checks for `load-file`, lookups, completions and malformed requests are
+coming next, along with the first release.
 [PLAN.md](PLAN.md) has all the details.
 
 ## Why "proof"?

doc/design.md

@@ -209,8 +209,9 @@ a small piece of Clojure. That's enough for the snippets of the nREPL
 profile, the code CIDER and vim-fireplace send when they connect and
 what client tests need (output, values, errors, input, something to
 interrupt), and it gives the same replies as nREPL 1.8.0 for the same
-code. Code in other languages wouldn't help, as no client's tests send
-Erlang to a server.
+code. The only exceptions are nREPL's bugs around interrupts, which
+proof serve has only as scenarios (e.g. `drop-queued`). Code in other
+languages wouldn't help, as no client's tests send Erlang to a server.
 
 Some rules are about what a client leaves behind - sessions that were
 never closed and `need-input` that was never answered. They apply only
@@ -305,6 +306,6 @@ to have a single test suite that every server can be checked against.
 ## Future Plans
 
 At this point proof covers the core of the protocol (`describe`, unknown
-ops, sessions, `eval`, `stdin` and the wire format), the requests of
+ops, sessions, `eval`, `stdin`, `interrupt` and the wire format), the requests of
 clients and the server differences clients have to deal with.
 [PLAN.md](../PLAN.md) has what's planned next.

doc/hacking.md

@@ -133,8 +133,9 @@ Here's what each of its fields means:
 | `Snippets`, `Needs` | The snippets and capabilities the check uses. Checks are skipped when the profile doesn't provide them. `t.Snippet` panics if a check uses a snippet that it didn't declare, which the tests catch, so the two can't get out of sync. |
 | `Requires` | The checks this check depends on. Everything that creates a session requires `session.clone`, and everything that looks at the response of `describe` requires `describe.reply`. |
 
-Checks are grouped by area in `describe.go`, `op.go`, `session.go` and
-`eval.go`, and they run in the order listed in `all.go`. If you're adding
+Checks are grouped by area in `describe.go`, `op.go`, `session.go`,
+`eval.go`, `stdin.go` and `interrupt.go`, and they run in the order
+listed in `all.go`. If you're adding
 a new area, put its checks in a new file and add them to `all.go`.
 
 In `Run` a check talks to the server via `t`:

doc/profiles.md

@@ -56,6 +56,15 @@ value = ":proof-marker"
 code = "1 2"
 values = ["1", "2"]
 
+[snippets.read-line]
+code = "(read-line)"
+value = '"proof"'
+eof = "nil"
+
+[snippets.sleep]
+code = '(do (print "proof") (flush) (Thread/sleep 60000))'
+out = "proof"
+
 [expected-failures]
 "eval.multiple-forms" = "only the last value is returned, see #42"
 ```
@@ -129,6 +138,7 @@ snippet is missing, all the checks that need it are skipped.
 | `session-state` | `code`, `use`, `value` | leave some state in the session that `use` reads back as `value` | `session.across-connections`, `session.isolated` |
 | `multiple` | `code`, `values` | consist of several forms that evaluate to `values` (in order) | `eval.multiple-forms` |
 | `read-line` | `code`, `value`, `eof` | read a line from stdin, evaluating to `value` when the line is `proof` and to `eof` at the end of input | `stdin.need-input`, `stdin.roundtrip`, `stdin.eof` |
+| `sleep` | `code`, `out` | print `out` and then keep running for longer than the `timeout` (e.g. a minute), so there's something to interrupt | all the `interrupt.*` checks |
 
 A few tips for writing snippets:
 
@@ -156,6 +166,9 @@ A few tips for writing snippets:
 
 - proof evaluates the `use` of `session-state` in a new session as well,
   where it must not return `value` (an error is fine).
+- proof interrupts `sleep` as soon as its `out` arrives, so make sure
+  your server sends the output right away (in Clojure that takes a
+  `flush`, as there's no newline).
 
 ## Expected Failures
 

doc/spec-changes.md

@@ -60,7 +60,23 @@ dialtone respond to the interrupt request with a plain `done` (or with
 latter two along with `error` in nREPL's case) and add `interrupted` and
 `done` to the response of the interrupted eval request. The spec's
 fallback when there's no `interrupt-id` ("the most recent request of the
-current session") also assumes sessions.
+current session") also assumes sessions. Conjure depends on that
+fallback, as it never sends `interrupt-id`.
+
+The spec doesn't say what happens around an interrupt either, and nREPL
+1.8.0 gets some of it wrong:
+
+- The evals waiting for the interrupted one should still run. nREPL
+  drops them ([nrepl#503](https://github.com/nrepl/nrepl/issues/503)), so
+  they never get their `done`, while CIDER sends evals while another one
+  is running all the time.
+- The `done` of the interrupted eval should go to the connection the
+  eval came from, even when the interrupt came from another one. nREPL
+  1.8.0 sends it to the interrupt's connection
+  ([nrepl#498](https://github.com/nrepl/nrepl/pull/498) fixes that).
+- Nothing should follow the `done` of the interrupted eval, but nREPL
+  sends the exception the interrupted code threw after it
+  ([nrepl#506](https://github.com/nrepl/nrepl/issues/506)).
 
 ### Reused Request IDs in the Examples
 

doc/usage.md

@@ -365,6 +365,9 @@ the scenarios, and every one of them is something a real server does
 | `ns-fallback` | An eval in a namespace that doesn't exist runs in the current one | jank |
 | `ns-error` | An eval in a namespace that doesn't exist fails without `namespace-not-found` | Basilisp |
 | `eof-error` | Reading past the end of input fails instead of returning `nil` | nREPL 1.7.0 |
+| `late-exception` | An interrupted eval's exception comes after its `done`, and the rest of its code still runs | nREPL 1.8.0 and Babashka |
+| `drop-queued` | Evals waiting for one that gets interrupted never run | nREPL 1.8.0 and Babashka |
+| `interrupt-elsewhere` | An interrupted eval's `done` goes to the connection the interrupt came from | nREPL 1.8.0 and Babashka |
 | `unsorted-keys` | The keys of reply dicts aren't sorted | jank |
 | `byte-writes` | Replies are written a byte at a time | any server |
 | `batched-writes` | Replies are held back and written together until the eval waits or ends | any server |
@@ -396,7 +399,9 @@ once for every scenario.
 
 proof can't evaluate real Clojure, of course. Instead it understands just
 enough of it for tests, and gives the same replies as nREPL 1.8.0 does
-for the same code:
+for the same code. The only exceptions are nREPL's bugs around
+interrupts (`late-exception`, `drop-queued` and `interrupt-elsewhere`),
+which you get with `-like clojure`:
 
 | Code | What it does |
 |---|---|

internal/checks/all.go

@@ -10,6 +10,7 @@ func All() []*check.Check {
 	all = append(all, sessionChecks()...)
 	all = append(all, evalChecks()...)
 	all = append(all, stdinChecks()...)
+	all = append(all, interruptChecks()...)
 	all = append(all, clientProfileChecks()...)
 	return all
 }

internal/checks/checks_test.go

@@ -26,6 +26,7 @@ var fakeProfile = &profile.Profile{
 		"session-state": {Code: "marker", Use: "*1", Value: ":proof-marker"},
 		"multiple":      {Code: "1 2", Values: []string{"1", "2"}},
 		"read-line":     {Code: "read", Value: `"proof"`, Eof: "nil"},
+		"sleep":         {Code: "sleep", Out: "proof"},
 	},
 }
 
@@ -51,17 +52,22 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 	// What every client but REPLy goes through first.
 	connects := map[string]check.Verdict{"cider.connect": F, "calva.connect": F, "conjure.connect": F, "fireplace.connect": F,
 		"neorepl.connect": F}
+	// The interrupt checks, which can't tell whether the server has the op.
+	noInterrupts := map[string]check.Verdict{}
+	for _, c := range interruptChecks() {
+		noInterrupts[c.ID] = S
+	}
 	cases := []struct {
 		name string
 		q    quirks
 		want map[string]check.Verdict
 	}{
 		// Conjure does without the features that need ops.
-		{"ops as a list", quirks{opsList: true}, checktest.Merged(connects, map[string]check.Verdict{"describe.ops-dict": F,
+		{"ops as a list", quirks{opsList: true}, checktest.Merged(connects, noInterrupts, map[string]check.Verdict{"describe.ops-dict": F,
 			"describe.required-ops": S, "stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S, "conjure.connect": check.Pass})},
 		{"clone not advertised", quirks{noClone: true}, map[string]check.Verdict{"describe.required-ops": F}},
 		{"no versions", quirks{noVersions: true}, map[string]check.Verdict{"describe.versions": W}},
-		{"describe kills the connection", quirks{crashOnDescribe: true}, checktest.Merged(connects, map[string]check.Verdict{
+		{"describe kills the connection", quirks{crashOnDescribe: true}, checktest.Merged(connects, noInterrupts, map[string]check.Verdict{
 			"describe.reply": F, "describe.ops-dict": S, "describe.required-ops": S, "describe.versions": S,
 			"stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S})},
 		{"no unknown-op", quirks{noUnknownOp: true}, map[string]check.Verdict{"op.unknown": F, "op.unknown-echo": W}},
@@ -76,7 +82,8 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 			"neorepl.eval": F}},
 		{"any session accepted", quirks{acceptAnySession: true}, map[string]check.Verdict{"session.unknown": F, "session.closed": F}},
 		{"shared session state", quirks{sharedState: true}, map[string]check.Verdict{"session.isolated": F}},
-		{"sessions tied to sockets", quirks{socketSessions: true}, map[string]check.Verdict{"session.across-connections": W}},
+		{"sessions tied to sockets", quirks{socketSessions: true}, map[string]check.Verdict{"session.across-connections": W,
+			"interrupt.other-connection": S}},
 		{"no ephemeral sessions", quirks{noEphemeral: true}, map[string]check.Verdict{"session.ephemeral": F, "fireplace.connect": F}},
 		{"last value only", quirks{lastValueOnly: true}, map[string]check.Verdict{"eval.multiple-forms": F, "fireplace.connect": F}},
 		{"stderr dropped", quirks{dropErr: true}, map[string]check.Verdict{"eval.stderr": F}},
@@ -102,7 +109,7 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		{"integer value", quirks{intValue: true}, map[string]check.Verdict{
 			"wire.field-types": F, "eval.value": F, "eval.survives-error": F, "eval.multiple-forms": F,
 			"session.ephemeral": F, "session.persistent": F, "cider.eval": F, "cider.repl": F, "calva.eval": F, "conjure.eval": F, "fireplace.eval": F,
-			"neorepl.eval": F}},
+			"neorepl.eval": F, "interrupt.survives": F, "interrupt.queued": F}},
 		{"unsorted keys", quirks{unsortedKeys: true}, map[string]check.Verdict{"wire.canonical": W}},
 		{"invalid UTF-8", quirks{badUTF8: true}, map[string]check.Verdict{"wire.utf8": W, "eval.stdout": F}},
 		{"no stdin op", quirks{noStdinOp: true}, map[string]check.Verdict{
@@ -114,6 +121,18 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 			"stdin.roundtrip": F, "stdin.eof": W}},
 		{"EOF is an error", quirks{eofError: true}, map[string]check.Verdict{"stdin.eof": W}},
 		{"input never arrives", quirks{dropStdin: true}, map[string]check.Verdict{"stdin.roundtrip": F, "stdin.eof": W}},
+		{"no interrupt op", quirks{noInterruptOp: true}, noInterrupts},
+		{"interrupt does nothing", quirks{interruptIgnored: true}, checktest.Merged(noInterrupts,
+			map[string]check.Verdict{"interrupt.running": F, "interrupt.idle": check.Pass})},
+		{"interrupt needs interrupt-id", quirks{needsInterruptID: true}, map[string]check.Verdict{"interrupt.no-id": F}},
+		{"no interrupted", quirks{noInterrupted: true}, map[string]check.Verdict{"interrupt.status": W}},
+		{"no session-idle", quirks{noSessionIdle: true}, map[string]check.Verdict{"interrupt.idle": W}},
+		{"interrupt-id ignored", quirks{anyInterruptID: true}, map[string]check.Verdict{"interrupt.id-mismatch": W}},
+		{"queued evals dropped", quirks{dropQueued: true}, map[string]check.Verdict{"interrupt.queued": F}},
+		// The interrupt's connection never sent the eval.
+		{"interrupted on the wrong connection", quirks{interruptHere: true}, map[string]check.Verdict{
+			"interrupt.other-connection": F, "wire.id": W}},
+		{"session dies on interrupt", quirks{diesOnInterrupt: true}, map[string]check.Verdict{"interrupt.survives": F, "interrupt.queued": F}},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {