Restartable syscalls, because retrying cannot win
`⚠ Could not reach the broker — Interrupted system call`. The new message earned its keep immediately: it said the HTTPS call and not the socket, which is what made this findable. Retrying was the wrong shape of fix and the arithmetic says why. The Dart VM's profiler samples every thread about a thousand times a second, and an HTTPS round trip takes far longer than a millisecond — so every attempt is interrupted and three attempts are three failures. A signal storm against a real request failed five times out of five, which is the reproduction the last two attempts at this could not manage. Two things had to change together, and the tests were run each way to be sure both are load-bearing. `SA_RESTART` is added to the handlers already installed for the signals that do this — SIGPROF, SIGALRM, SIGVTALRM, SIGCHLD. The handler itself is untouched: same function, same mask. The profiler still gets its signal and still samples; the read underneath carries on instead of failing. And the HTTPS clients no longer set a socket timeout. Linux never restarts a socket call that has one, whatever SA_RESTART says, so a timeout meant every request died on the first signal regardless. The cost is real and is written down where the client is made: a server that accepts and then says nothing now holds the call until TCP gives up. Two smaller things found on the way. `Response.body` is a stream read where it is first touched, so reading it at `parseJson` put the longest blocking part of a round trip outside the retry meant to cover it; and a client whose handshake was cut short is not a client to ask again, so each attempt now builds its own. The first version of the flag helper read the handler with Nim's `sigaction`, which takes the new action where C takes a NULL — so it replaced the handler with SIG_DFL rather than reading it, and the next signal killed the process. It imports C's own shape now, and there is a test that the handler comes back unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dcd31d9 parent: 4925e54 modified
nim/src/frq/atproto.nim +33 -14 | @@ -41,8 +41,12 @@ proc b64urlDecode*(s: string): string = | ||
| 41 | 41 | try: decode(t) except CatchableError: "" |
| 42 | 42 | |
| 43 | 43 | proc newClient(): HttpClient = |
| 44 | - newHttpClient(timeout = 15_000, | |
| 45 | - sslContext = newContext(verifyMode = CVerifyPeer)) | |
| 44 | + ## No socket timeout, and that is not an oversight: Linux never restarts a | |
| 45 | + ## socket call that has one, whatever `SA_RESTART` says, so a timeout here | |
| 46 | + ## means every request dies on the first signal to arrive. See `frq/eintr`. | |
| 47 | + ## The cost is that a server which accepts and then says nothing holds the | |
| 48 | + ## call until TCP gives up. | |
| 49 | + newHttpClient(sslContext = newContext(verifyMode = CVerifyPeer)) | |
| 46 | 50 | |
| 47 | 51 | proc getJson(url, whatFor: string): JsonNode = |
| 48 | 52 | ## `whatFor` is what the caller was trying to do, because the failure the |
| @@ -50,11 +54,16 @@ proc getJson(url, whatFor: string): JsonNode = | ||
| 50 | 54 | ## directory answers an unknown handle with a bare 400, and "400 Bad |
| 51 | 55 | ## Request" on the connect screen tells nobody anything. |
| 52 | 56 | trace("atproto", "GET " & url) |
| 53 | - let c = newClient() | |
| 57 | + var c: HttpClient | |
| 54 | 58 | try: |
| 55 | 59 | var body: string |
| 56 | - # Retried where a signal cut the round trip short; see `frq/eintr`. | |
| 60 | + # A client per attempt: one whose handshake a signal cut short is not one | |
| 61 | + # to ask again. `getContent` reads the whole body, so unlike `request` | |
| 62 | + # there is nothing left outside the retry. See `frq/eintr`. | |
| 57 | 63 | retrying 3: |
| 64 | + if c != nil: | |
| 65 | + try: c.close() except CatchableError: discard | |
| 66 | + c = newClient() | |
| 58 | 67 | body = c.getContent(url) |
| 59 | 68 | parseJson(body) |
| 60 | 69 | except JsonParsingError: |
| @@ -63,7 +72,8 @@ proc getJson(url, whatFor: string): JsonNode = | ||
| 63 | 72 | trace("atproto", "!! " & e.msg) |
| 64 | 73 | raise newException(AtprotoError, whatFor) |
| 65 | 74 | finally: |
| 66 | - c.close() | |
| 75 | + if c != nil: | |
| 76 | + try: c.close() except CatchableError: discard | |
| 67 | 77 | |
| 68 | 78 | func hostOf*(url: string): string = |
| 69 | 79 | var u = url |
| @@ -129,21 +139,29 @@ proc createSession*(handle, password: string): Session = | ||
| 129 | 139 | let pds = pdsFor(did) |
| 130 | 140 | trace("atproto", "createSession at " & pds) |
| 131 | 141 | |
| 132 | - let c = newClient() | |
| 142 | + var c: HttpClient | |
| 133 | 143 | try: |
| 134 | - c.headers = newHttpHeaders({"Content-Type": "application/json"}) | |
| 135 | 144 | let payload = $(%*{"identifier": handle.strip(), "password": password}) |
| 136 | - var res: Response | |
| 145 | + var status, raw: string | |
| 137 | 146 | retrying 3: |
| 138 | - res = c.request("https://" & hostOf(pds) & | |
| 139 | - "/xrpc/com.atproto.server.createSession", | |
| 140 | - httpMethod = HttpPost, body = payload) | |
| 147 | + if c != nil: | |
| 148 | + try: c.close() except CatchableError: discard | |
| 149 | + c = newClient() | |
| 150 | + c.headers = newHttpHeaders({"Content-Type": "application/json"}) | |
| 151 | + let res = c.request("https://" & hostOf(pds) & | |
| 152 | + "/xrpc/com.atproto.server.createSession", | |
| 153 | + httpMethod = HttpPost, body = payload) | |
| 154 | + # Inside the retry: `Response.body` is a stream read where it is first | |
| 155 | + # touched, so reading it below would put the longest blocking part of | |
| 156 | + # the round trip outside the retry meant to cover it. | |
| 157 | + status = res.status | |
| 158 | + raw = res.body | |
| 141 | 159 | # Read the body whatever the status: the PDS puts the reason in it, and a |
| 142 | 160 | # wrong app password is a 401 whose message is the useful part. |
| 143 | - let body = try: parseJson(res.body) | |
| 161 | + let body = try: parseJson(raw) | |
| 144 | 162 | except JsonParsingError: |
| 145 | 163 | raise newException(AtprotoError, |
| 146 | - "Your PDS refused the sign-in (" & res.status & ").") | |
| 164 | + "Your PDS refused the sign-in (" & status & ").") | |
| 147 | 165 | let jwt = body{"accessJwt"}.getStr() |
| 148 | 166 | if jwt.len == 0: |
| 149 | 167 | let msg = body{"message"}.getStr() |
| @@ -156,7 +174,8 @@ proc createSession*(handle, password: string): Session = | ||
| 156 | 174 | accessJwt: jwt, |
| 157 | 175 | pds: pds) |
| 158 | 176 | finally: |
| 159 | - c.close() | |
| 177 | + if c != nil: | |
| 178 | + try: c.close() except CatchableError: discard | |
| 160 | 179 | |
| 161 | 180 | proc saslResponse*(s: Session, nonce: string): string = |
| 162 | 181 | ## The base64url SASL payload, for either kind freeq takes. |
| @@ -41,8 +41,12 @@ proc b64urlDecode*(s: string): string = | |||
| 41 | try: decode(t) except CatchableError: "" | 41 | try: decode(t) except CatchableError: "" |
| 42 | 42 | ||
| 43 | proc newClient(): HttpClient = | 43 | proc newClient(): HttpClient = |
| 44 | - newHttpClient(timeout = 15_000, | 44 | + ## No socket timeout, and that is not an oversight: Linux never restarts a |
| 45 | - sslContext = newContext(verifyMode = CVerifyPeer)) | 45 | + ## socket call that has one, whatever `SA_RESTART` says, so a timeout here |
| 46 | + ## means every request dies on the first signal to arrive. See `frq/eintr`. | ||
| 47 | + ## The cost is that a server which accepts and then says nothing holds the | ||
| 48 | + ## call until TCP gives up. | ||
| 49 | + newHttpClient(sslContext = newContext(verifyMode = CVerifyPeer)) | ||
| 46 | 50 | ||
| 47 | proc getJson(url, whatFor: string): JsonNode = | 51 | proc getJson(url, whatFor: string): JsonNode = |
| 48 | ## `whatFor` is what the caller was trying to do, because the failure the | 52 | ## `whatFor` is what the caller was trying to do, because the failure the |
| @@ -50,11 +54,16 @@ proc getJson(url, whatFor: string): JsonNode = | |||
| 50 | ## directory answers an unknown handle with a bare 400, and "400 Bad | 54 | ## directory answers an unknown handle with a bare 400, and "400 Bad |
| 51 | ## Request" on the connect screen tells nobody anything. | 55 | ## Request" on the connect screen tells nobody anything. |
| 52 | trace("atproto", "GET " & url) | 56 | trace("atproto", "GET " & url) |
| 53 | - let c = newClient() | 57 | + var c: HttpClient |
| 54 | try: | 58 | try: |
| 55 | var body: string | 59 | var body: string |
| 56 | - # Retried where a signal cut the round trip short; see `frq/eintr`. | 60 | + # A client per attempt: one whose handshake a signal cut short is not one |
| 61 | + # to ask again. `getContent` reads the whole body, so unlike `request` | ||
| 62 | + # there is nothing left outside the retry. See `frq/eintr`. | ||
| 57 | retrying 3: | 63 | retrying 3: |
| 64 | + if c != nil: | ||
| 65 | + try: c.close() except CatchableError: discard | ||
| 66 | + c = newClient() | ||
| 58 | body = c.getContent(url) | 67 | body = c.getContent(url) |
| 59 | parseJson(body) | 68 | parseJson(body) |
| 60 | except JsonParsingError: | 69 | except JsonParsingError: |
| @@ -63,7 +72,8 @@ proc getJson(url, whatFor: string): JsonNode = | |||
| 63 | trace("atproto", "!! " & e.msg) | 72 | trace("atproto", "!! " & e.msg) |
| 64 | raise newException(AtprotoError, whatFor) | 73 | raise newException(AtprotoError, whatFor) |
| 65 | finally: | 74 | finally: |
| 66 | - c.close() | 75 | + if c != nil: |
| 76 | + try: c.close() except CatchableError: discard | ||
| 67 | 77 | ||
| 68 | func hostOf*(url: string): string = | 78 | func hostOf*(url: string): string = |
| 69 | var u = url | 79 | var u = url |
| @@ -129,21 +139,29 @@ proc createSession*(handle, password: string): Session = | |||
| 129 | let pds = pdsFor(did) | 139 | let pds = pdsFor(did) |
| 130 | trace("atproto", "createSession at " & pds) | 140 | trace("atproto", "createSession at " & pds) |
| 131 | 141 | ||
| 132 | - let c = newClient() | 142 | + var c: HttpClient |
| 133 | try: | 143 | try: |
| 134 | - c.headers = newHttpHeaders({"Content-Type": "application/json"}) | ||
| 135 | let payload = $(%*{"identifier": handle.strip(), "password": password}) | 144 | let payload = $(%*{"identifier": handle.strip(), "password": password}) |
| 136 | - var res: Response | 145 | + var status, raw: string |
| 137 | retrying 3: | 146 | retrying 3: |
| 138 | - res = c.request("https://" & hostOf(pds) & | 147 | + if c != nil: |
| 139 | - "/xrpc/com.atproto.server.createSession", | 148 | + try: c.close() except CatchableError: discard |
| 140 | - httpMethod = HttpPost, body = payload) | 149 | + c = newClient() |
| 150 | + c.headers = newHttpHeaders({"Content-Type": "application/json"}) | ||
| 151 | + let res = c.request("https://" & hostOf(pds) & | ||
| 152 | + "/xrpc/com.atproto.server.createSession", | ||
| 153 | + httpMethod = HttpPost, body = payload) | ||
| 154 | + # Inside the retry: `Response.body` is a stream read where it is first | ||
| 155 | + # touched, so reading it below would put the longest blocking part of | ||
| 156 | + # the round trip outside the retry meant to cover it. | ||
| 157 | + status = res.status | ||
| 158 | + raw = res.body | ||
| 141 | # Read the body whatever the status: the PDS puts the reason in it, and a | 159 | # Read the body whatever the status: the PDS puts the reason in it, and a |
| 142 | # wrong app password is a 401 whose message is the useful part. | 160 | # wrong app password is a 401 whose message is the useful part. |
| 143 | - let body = try: parseJson(res.body) | 161 | + let body = try: parseJson(raw) |
| 144 | except JsonParsingError: | 162 | except JsonParsingError: |
| 145 | raise newException(AtprotoError, | 163 | raise newException(AtprotoError, |
| 146 | - "Your PDS refused the sign-in (" & res.status & ").") | 164 | + "Your PDS refused the sign-in (" & status & ").") |
| 147 | let jwt = body{"accessJwt"}.getStr() | 165 | let jwt = body{"accessJwt"}.getStr() |
| 148 | if jwt.len == 0: | 166 | if jwt.len == 0: |
| 149 | let msg = body{"message"}.getStr() | 167 | let msg = body{"message"}.getStr() |
| @@ -156,7 +174,8 @@ proc createSession*(handle, password: string): Session = | |||
| 156 | accessJwt: jwt, | 174 | accessJwt: jwt, |
| 157 | pds: pds) | 175 | pds: pds) |
| 158 | finally: | 176 | finally: |
| 159 | - c.close() | 177 | + if c != nil: |
| 178 | + try: c.close() except CatchableError: discard | ||
| 160 | 179 | ||
| 161 | proc saslResponse*(s: Session, nonce: string): string = | 180 | proc saslResponse*(s: Session, nonce: string): string = |
| 162 | ## The base64url SASL payload, for either kind freeq takes. | 181 | ## The base64url SASL payload, for either kind freeq takes. |
modified
nim/src/frq/eintr.nim +50 -2 | @@ -26,9 +26,12 @@ func interrupted*(e: ref CatchableError): bool = | ||
| 26 | 26 | ## By `errorCode` where there is one, and by message otherwise: a library |
| 27 | 27 | ## that catches an OSError and re-raises its text — `httpclient` does this |
| 28 | 28 | ## in places — loses the code but keeps the words. |
| 29 | - if e of OSError: | |
| 30 | - ((ref OSError)(e)).errorCode.int32 == p.EINTR.int32 | |
| 29 | + if e of OSError and ((ref OSError)(e)).errorCode.int32 == p.EINTR.int32: | |
| 30 | + true | |
| 31 | 31 | else: |
| 32 | + # The message as well as the code, and for an OSError too: a code of zero | |
| 33 | + # with those words in the text is a wrapper that lost the errno on the | |
| 34 | + # way, not a different failure. | |
| 32 | 35 | "Interrupted system call" in e.msg |
| 33 | 36 | |
| 34 | 37 | template retrying*(attempts: int, body: untyped): untyped = |
| @@ -45,3 +48,48 @@ template retrying*(attempts: int, body: untyped): untyped = | ||
| 45 | 48 | except CatchableError as e: |
| 46 | 49 | if tries < attempts and interrupted(e): continue |
| 47 | 50 | raise |
| 51 | + | |
| 52 | +# ------------------------------------------------- asking for restartable calls | |
| 53 | + | |
| 54 | +proc sigactionRaw(sig: cint, act, old: ptr Sigaction): cint | |
| 55 | + {.importc: "sigaction", header: "<signal.h>".} | |
| 56 | + ## C's own shape, because the question here is "what is installed?" and | |
| 57 | + ## Nim's binding takes the new action where C takes a NULL. Passing an | |
| 58 | + ## uninitialised struct there does not read the handler, it replaces it | |
| 59 | + ## with SIG_DFL — and the first signal after that killed the process, | |
| 60 | + ## which is how this line came to be written. | |
| 61 | + | |
| 62 | +proc sigactionOf*(sig: cint, into: var Sigaction): bool = | |
| 63 | + ## What is installed for `sig`, without changing it. Exported for the test | |
| 64 | + ## that checks this module leaves a handler as it found it. | |
| 65 | + sigactionRaw(sig, nil, addr into) == 0 | |
| 66 | + | |
| 67 | +proc restartOn(sig: cint) = | |
| 68 | + ## Add `SA_RESTART` to whatever handler is already installed for `sig`. | |
| 69 | + ## | |
| 70 | + ## The handler itself is not touched — same function, same mask. The flag | |
| 71 | + ## says only what the kernel should do to a syscall the signal lands in: | |
| 72 | + ## restart it rather than fail it with EINTR. | |
| 73 | + var current: Sigaction | |
| 74 | + if sigactionRaw(sig, nil, addr current) != 0: return | |
| 75 | + if (current.sa_flags and p.SA_RESTART) != 0: return | |
| 76 | + var updated = current | |
| 77 | + updated.sa_flags = current.sa_flags or p.SA_RESTART | |
| 78 | + discard sigactionRaw(sig, addr updated, nil) | |
| 79 | + | |
| 80 | +proc restartableSyscalls*() = | |
| 81 | + ## Ask for interrupted syscalls to be restarted rather than reported. | |
| 82 | + ## | |
| 83 | + ## Retrying is not enough on its own, and the reason is arithmetic: the | |
| 84 | + ## Dart VM's profiler samples every thread about a thousand times a second, | |
| 85 | + ## and an HTTPS round trip takes far longer than a millisecond. Every | |
| 86 | + ## attempt is interrupted, so three attempts are three failures — which is | |
| 87 | + ## what a signal storm against a real request showed, five times out of | |
| 88 | + ## five. | |
| 89 | + ## | |
| 90 | + ## So the flag, which costs nothing and changes nobody's handler. The | |
| 91 | + ## profiler still gets its signal and still samples; the read underneath | |
| 92 | + ## carries on rather than failing. The retries stay, for a signal arriving | |
| 93 | + ## from somewhere this never reached. | |
| 94 | + for sig in [p.SIGPROF, p.SIGALRM, p.SIGVTALRM, p.SIGCHLD]: | |
| 95 | + restartOn(sig) | |
| @@ -26,9 +26,12 @@ func interrupted*(e: ref CatchableError): bool = | |||
| 26 | ## By `errorCode` where there is one, and by message otherwise: a library | 26 | ## By `errorCode` where there is one, and by message otherwise: a library |
| 27 | ## that catches an OSError and re-raises its text — `httpclient` does this | 27 | ## that catches an OSError and re-raises its text — `httpclient` does this |
| 28 | ## in places — loses the code but keeps the words. | 28 | ## in places — loses the code but keeps the words. |
| 29 | - if e of OSError: | 29 | + if e of OSError and ((ref OSError)(e)).errorCode.int32 == p.EINTR.int32: |
| 30 | - ((ref OSError)(e)).errorCode.int32 == p.EINTR.int32 | 30 | + true |
| 31 | else: | 31 | else: |
| 32 | + # The message as well as the code, and for an OSError too: a code of zero | ||
| 33 | + # with those words in the text is a wrapper that lost the errno on the | ||
| 34 | + # way, not a different failure. | ||
| 32 | "Interrupted system call" in e.msg | 35 | "Interrupted system call" in e.msg |
| 33 | 36 | ||
| 34 | template retrying*(attempts: int, body: untyped): untyped = | 37 | template retrying*(attempts: int, body: untyped): untyped = |
| @@ -45,3 +48,48 @@ template retrying*(attempts: int, body: untyped): untyped = | |||
| 45 | except CatchableError as e: | 48 | except CatchableError as e: |
| 46 | if tries < attempts and interrupted(e): continue | 49 | if tries < attempts and interrupted(e): continue |
| 47 | raise | 50 | raise |
| 51 | + | ||
| 52 | +# ------------------------------------------------- asking for restartable calls | ||
| 53 | + | ||
| 54 | +proc sigactionRaw(sig: cint, act, old: ptr Sigaction): cint | ||
| 55 | + {.importc: "sigaction", header: "<signal.h>".} | ||
| 56 | + ## C's own shape, because the question here is "what is installed?" and | ||
| 57 | + ## Nim's binding takes the new action where C takes a NULL. Passing an | ||
| 58 | + ## uninitialised struct there does not read the handler, it replaces it | ||
| 59 | + ## with SIG_DFL — and the first signal after that killed the process, | ||
| 60 | + ## which is how this line came to be written. | ||
| 61 | + | ||
| 62 | +proc sigactionOf*(sig: cint, into: var Sigaction): bool = | ||
| 63 | + ## What is installed for `sig`, without changing it. Exported for the test | ||
| 64 | + ## that checks this module leaves a handler as it found it. | ||
| 65 | + sigactionRaw(sig, nil, addr into) == 0 | ||
| 66 | + | ||
| 67 | +proc restartOn(sig: cint) = | ||
| 68 | + ## Add `SA_RESTART` to whatever handler is already installed for `sig`. | ||
| 69 | + ## | ||
| 70 | + ## The handler itself is not touched — same function, same mask. The flag | ||
| 71 | + ## says only what the kernel should do to a syscall the signal lands in: | ||
| 72 | + ## restart it rather than fail it with EINTR. | ||
| 73 | + var current: Sigaction | ||
| 74 | + if sigactionRaw(sig, nil, addr current) != 0: return | ||
| 75 | + if (current.sa_flags and p.SA_RESTART) != 0: return | ||
| 76 | + var updated = current | ||
| 77 | + updated.sa_flags = current.sa_flags or p.SA_RESTART | ||
| 78 | + discard sigactionRaw(sig, addr updated, nil) | ||
| 79 | + | ||
| 80 | +proc restartableSyscalls*() = | ||
| 81 | + ## Ask for interrupted syscalls to be restarted rather than reported. | ||
| 82 | + ## | ||
| 83 | + ## Retrying is not enough on its own, and the reason is arithmetic: the | ||
| 84 | + ## Dart VM's profiler samples every thread about a thousand times a second, | ||
| 85 | + ## and an HTTPS round trip takes far longer than a millisecond. Every | ||
| 86 | + ## attempt is interrupted, so three attempts are three failures — which is | ||
| 87 | + ## what a signal storm against a real request showed, five times out of | ||
| 88 | + ## five. | ||
| 89 | + ## | ||
| 90 | + ## So the flag, which costs nothing and changes nobody's handler. The | ||
| 91 | + ## profiler still gets its signal and still samples; the read underneath | ||
| 92 | + ## carries on rather than failing. The retries stay, for a signal arriving | ||
| 93 | + ## from somewhere this never reached. | ||
| 94 | + for sig in [p.SIGPROF, p.SIGALRM, p.SIGVTALRM, p.SIGCHLD]: | ||
| 95 | + restartOn(sig) | ||
modified
nim/src/frq/oauth.nim +26 -10 | @@ -115,21 +115,36 @@ proc refreshSession*(broker, brokerToken: string): Tokens = | ||
| 115 | 115 | ## This is what a second run uses: the token that came back through the |
| 116 | 116 | ## browser was spent on the first connection, and the reader should not see |
| 117 | 117 | ## a login page again for it. |
| 118 | - let c = newHttpClient(timeout = 15_000, | |
| 119 | - sslContext = newContext(verifyMode = CVerifyPeer)) | |
| 118 | + # A client per attempt, and the body read inside it. | |
| 119 | + # | |
| 120 | + # Neither of those is fussiness. A client whose handshake was cut short is | |
| 121 | + # not a client to ask again — the retry rides a half-open connection — and | |
| 122 | + # `Response.body` is a stream that is read where it is first touched, so | |
| 123 | + # reading it at `parseJson` put the longest blocking call of the round trip | |
| 124 | + # outside the very retry meant to cover it. `⚠ Could not reach the broker — | |
| 125 | + # Interrupted system call` survived the first fix for that reason. | |
| 126 | + var | |
| 127 | + c: HttpClient | |
| 128 | + status: string | |
| 129 | + body: string | |
| 120 | 130 | try: |
| 121 | - c.headers = newHttpHeaders({"Content-Type": "application/json"}) | |
| 122 | - var res: Response | |
| 123 | 131 | retrying 3: |
| 124 | - res = c.request("https://" & brokerHost(broker) & "/session", | |
| 125 | - httpMethod = HttpPost, | |
| 126 | - body = $(%*{"broker_token": brokerToken})) | |
| 132 | + if c != nil: | |
| 133 | + try: c.close() except CatchableError: discard | |
| 134 | + # No socket timeout, deliberately; see `frq/eintr`. | |
| 135 | + c = newHttpClient(sslContext = newContext(verifyMode = CVerifyPeer)) | |
| 136 | + c.headers = newHttpHeaders({"Content-Type": "application/json"}) | |
| 137 | + let res = c.request("https://" & brokerHost(broker) & "/session", | |
| 138 | + httpMethod = HttpPost, | |
| 139 | + body = $(%*{"broker_token": brokerToken})) | |
| 140 | + status = res.status | |
| 141 | + body = res.body | |
| 127 | 142 | # The body whatever the status: an expired token is a 401 whose message |
| 128 | 143 | # is the part worth showing. |
| 129 | - let j = try: parseJson(res.body) | |
| 144 | + let j = try: parseJson(body) | |
| 130 | 145 | except CatchableError: |
| 131 | 146 | raise newException(OauthError, |
| 132 | - "The broker's answer was not JSON (" & res.status & ").") | |
| 147 | + "The broker's answer was not JSON (" & status & ").") | |
| 133 | 148 | let token = j{"token"}.getStr() |
| 134 | 149 | if token.len == 0: |
| 135 | 150 | let m = j{"message"}.getStr() |
| @@ -139,7 +154,8 @@ proc refreshSession*(broker, brokerToken: string): Tokens = | ||
| 139 | 154 | nick: j{"nick"}.getStr(), did: j{"did"}.getStr(), |
| 140 | 155 | handle: j{"handle"}.getStr()) |
| 141 | 156 | finally: |
| 142 | - c.close() | |
| 157 | + if c != nil: | |
| 158 | + try: c.close() except CatchableError: discard | |
| 143 | 159 | |
| 144 | 160 | # --------------------------------------------------------- the capture page |
| 145 | 161 | |
| @@ -115,21 +115,36 @@ proc refreshSession*(broker, brokerToken: string): Tokens = | |||
| 115 | ## This is what a second run uses: the token that came back through the | 115 | ## This is what a second run uses: the token that came back through the |
| 116 | ## browser was spent on the first connection, and the reader should not see | 116 | ## browser was spent on the first connection, and the reader should not see |
| 117 | ## a login page again for it. | 117 | ## a login page again for it. |
| 118 | - let c = newHttpClient(timeout = 15_000, | 118 | + # A client per attempt, and the body read inside it. |
| 119 | - sslContext = newContext(verifyMode = CVerifyPeer)) | 119 | + # |
| 120 | + # Neither of those is fussiness. A client whose handshake was cut short is | ||
| 121 | + # not a client to ask again — the retry rides a half-open connection — and | ||
| 122 | + # `Response.body` is a stream that is read where it is first touched, so | ||
| 123 | + # reading it at `parseJson` put the longest blocking call of the round trip | ||
| 124 | + # outside the very retry meant to cover it. `⚠ Could not reach the broker — | ||
| 125 | + # Interrupted system call` survived the first fix for that reason. | ||
| 126 | + var | ||
| 127 | + c: HttpClient | ||
| 128 | + status: string | ||
| 129 | + body: string | ||
| 120 | try: | 130 | try: |
| 121 | - c.headers = newHttpHeaders({"Content-Type": "application/json"}) | ||
| 122 | - var res: Response | ||
| 123 | retrying 3: | 131 | retrying 3: |
| 124 | - res = c.request("https://" & brokerHost(broker) & "/session", | 132 | + if c != nil: |
| 125 | - httpMethod = HttpPost, | 133 | + try: c.close() except CatchableError: discard |
| 126 | - body = $(%*{"broker_token": brokerToken})) | 134 | + # No socket timeout, deliberately; see `frq/eintr`. |
| 135 | + c = newHttpClient(sslContext = newContext(verifyMode = CVerifyPeer)) | ||
| 136 | + c.headers = newHttpHeaders({"Content-Type": "application/json"}) | ||
| 137 | + let res = c.request("https://" & brokerHost(broker) & "/session", | ||
| 138 | + httpMethod = HttpPost, | ||
| 139 | + body = $(%*{"broker_token": brokerToken})) | ||
| 140 | + status = res.status | ||
| 141 | + body = res.body | ||
| 127 | # The body whatever the status: an expired token is a 401 whose message | 142 | # The body whatever the status: an expired token is a 401 whose message |
| 128 | # is the part worth showing. | 143 | # is the part worth showing. |
| 129 | - let j = try: parseJson(res.body) | 144 | + let j = try: parseJson(body) |
| 130 | except CatchableError: | 145 | except CatchableError: |
| 131 | raise newException(OauthError, | 146 | raise newException(OauthError, |
| 132 | - "The broker's answer was not JSON (" & res.status & ").") | 147 | + "The broker's answer was not JSON (" & status & ").") |
| 133 | let token = j{"token"}.getStr() | 148 | let token = j{"token"}.getStr() |
| 134 | if token.len == 0: | 149 | if token.len == 0: |
| 135 | let m = j{"message"}.getStr() | 150 | let m = j{"message"}.getStr() |
| @@ -139,7 +154,8 @@ proc refreshSession*(broker, brokerToken: string): Tokens = | |||
| 139 | nick: j{"nick"}.getStr(), did: j{"did"}.getStr(), | 154 | nick: j{"nick"}.getStr(), did: j{"did"}.getStr(), |
| 140 | handle: j{"handle"}.getStr()) | 155 | handle: j{"handle"}.getStr()) |
| 141 | finally: | 156 | finally: |
| 142 | - c.close() | 157 | + if c != nil: |
| 158 | + try: c.close() except CatchableError: discard | ||
| 143 | 159 | ||
| 144 | # --------------------------------------------------------- the capture page | 160 | # --------------------------------------------------------- the capture page |
| 145 | 161 | ||
modified
nim/src/frq_core.nim +6 -1 | @@ -26,7 +26,7 @@ | ||
| 26 | 26 | ## between this and the experiment that was deleted for being a facsimile. |
| 27 | 27 | |
| 28 | 28 | import std/[json, strutils, tables] |
| 29 | -import frq/[ircparse, trace, ui, cells, reducer, model, rooms] | |
| 29 | +import frq/[ircparse, trace, ui, cells, reducer, model, rooms, eintr] | |
| 30 | 30 | import frq/conn as tr |
| 31 | 31 | import frq/screens/connect as scConnectScreen |
| 32 | 32 | import frq/screens/chats as scChatsScreen |
| @@ -52,6 +52,11 @@ proc frq_init*() {.exportc, dynlib.} = | ||
| 52 | 52 | ## `frq_ui_reset` deliberately does not do this. It is the tests' entry |
| 53 | 53 | ## point, and a suite that picked up whoever is signed in on the machine |
| 54 | 54 | ## running it would pass or fail by accident. |
| 55 | + # Before anything opens a socket. The Dart VM's profiler signals every | |
| 56 | + # thread in this process about a thousand times a second, and a syscall | |
| 57 | + # interrupted by one fails rather than resuming unless its handler says | |
| 58 | + # otherwise — which is why signing in reported `Interrupted system call`. | |
| 59 | + restartableSyscalls() | |
| 55 | 60 | reducer.restore() |
| 56 | 61 | |
| 57 | 62 | proc dup(s: string): cstring = |
| @@ -26,7 +26,7 @@ | |||
| 26 | ## between this and the experiment that was deleted for being a facsimile. | 26 | ## between this and the experiment that was deleted for being a facsimile. |
| 27 | 27 | ||
| 28 | import std/[json, strutils, tables] | 28 | import std/[json, strutils, tables] |
| 29 | -import frq/[ircparse, trace, ui, cells, reducer, model, rooms] | 29 | +import frq/[ircparse, trace, ui, cells, reducer, model, rooms, eintr] |
| 30 | import frq/conn as tr | 30 | import frq/conn as tr |
| 31 | import frq/screens/connect as scConnectScreen | 31 | import frq/screens/connect as scConnectScreen |
| 32 | import frq/screens/chats as scChatsScreen | 32 | import frq/screens/chats as scChatsScreen |
| @@ -52,6 +52,11 @@ proc frq_init*() {.exportc, dynlib.} = | |||
| 52 | ## `frq_ui_reset` deliberately does not do this. It is the tests' entry | 52 | ## `frq_ui_reset` deliberately does not do this. It is the tests' entry |
| 53 | ## point, and a suite that picked up whoever is signed in on the machine | 53 | ## point, and a suite that picked up whoever is signed in on the machine |
| 54 | ## running it would pass or fail by accident. | 54 | ## running it would pass or fail by accident. |
| 55 | + # Before anything opens a socket. The Dart VM's profiler signals every | ||
| 56 | + # thread in this process about a thousand times a second, and a syscall | ||
| 57 | + # interrupted by one fails rather than resuming unless its handler says | ||
| 58 | + # otherwise — which is why signing in reported `Interrupted system call`. | ||
| 59 | + restartableSyscalls() | ||
| 55 | reducer.restore() | 60 | reducer.restore() |
| 56 | 61 | ||
| 57 | proc dup(s: string): cstring = | 62 | proc dup(s: string): cstring = |
modified
nim/tests/teintr.nim +32 -0 | @@ -56,3 +56,35 @@ suite "retrying": | ||
| 56 | 56 | runs.inc |
| 57 | 57 | raise osError(p.EINTR, "Interrupted system call") |
| 58 | 58 | check runs == 4 |
| 59 | + | |
| 60 | +suite "restartableSyscalls": | |
| 61 | + test "adds SA_RESTART without disturbing the handler": | |
| 62 | + # The flag says what the kernel does to an interrupted syscall. It must | |
| 63 | + # not change which function runs, or whose signal it is — the profiler | |
| 64 | + # whose handler this finds is somebody else's. | |
| 65 | + proc handler(sig: cint) {.noconv.} = discard | |
| 66 | + var wanted: Sigaction | |
| 67 | + discard sigemptyset(wanted.sa_mask) | |
| 68 | + wanted.sa_handler = handler | |
| 69 | + wanted.sa_flags = 0 | |
| 70 | + check p.sigaction(SIGALRM, wanted, nil) == 0 | |
| 71 | + | |
| 72 | + restartableSyscalls() | |
| 73 | + | |
| 74 | + var got: Sigaction | |
| 75 | + check sigactionOf(SIGALRM, got) | |
| 76 | + check (got.sa_flags and p.SA_RESTART) != 0 | |
| 77 | + check got.sa_handler == handler | |
| 78 | + | |
| 79 | + test "and leaves one that already has it alone": | |
| 80 | + proc handler(sig: cint) {.noconv.} = discard | |
| 81 | + var wanted: Sigaction | |
| 82 | + discard sigemptyset(wanted.sa_mask) | |
| 83 | + wanted.sa_handler = handler | |
| 84 | + wanted.sa_flags = p.SA_RESTART | |
| 85 | + check p.sigaction(SIGALRM, wanted, nil) == 0 | |
| 86 | + restartableSyscalls() | |
| 87 | + var got: Sigaction | |
| 88 | + check sigactionOf(SIGALRM, got) | |
| 89 | + check got.sa_handler == handler | |
| 90 | + check (got.sa_flags and p.SA_RESTART) != 0 | |
| @@ -56,3 +56,35 @@ suite "retrying": | |||
| 56 | runs.inc | 56 | runs.inc |
| 57 | raise osError(p.EINTR, "Interrupted system call") | 57 | raise osError(p.EINTR, "Interrupted system call") |
| 58 | check runs == 4 | 58 | check runs == 4 |
| 59 | + | ||
| 60 | +suite "restartableSyscalls": | ||
| 61 | + test "adds SA_RESTART without disturbing the handler": | ||
| 62 | + # The flag says what the kernel does to an interrupted syscall. It must | ||
| 63 | + # not change which function runs, or whose signal it is — the profiler | ||
| 64 | + # whose handler this finds is somebody else's. | ||
| 65 | + proc handler(sig: cint) {.noconv.} = discard | ||
| 66 | + var wanted: Sigaction | ||
| 67 | + discard sigemptyset(wanted.sa_mask) | ||
| 68 | + wanted.sa_handler = handler | ||
| 69 | + wanted.sa_flags = 0 | ||
| 70 | + check p.sigaction(SIGALRM, wanted, nil) == 0 | ||
| 71 | + | ||
| 72 | + restartableSyscalls() | ||
| 73 | + | ||
| 74 | + var got: Sigaction | ||
| 75 | + check sigactionOf(SIGALRM, got) | ||
| 76 | + check (got.sa_flags and p.SA_RESTART) != 0 | ||
| 77 | + check got.sa_handler == handler | ||
| 78 | + | ||
| 79 | + test "and leaves one that already has it alone": | ||
| 80 | + proc handler(sig: cint) {.noconv.} = discard | ||
| 81 | + var wanted: Sigaction | ||
| 82 | + discard sigemptyset(wanted.sa_mask) | ||
| 83 | + wanted.sa_handler = handler | ||
| 84 | + wanted.sa_flags = p.SA_RESTART | ||
| 85 | + check p.sigaction(SIGALRM, wanted, nil) == 0 | ||
| 86 | + restartableSyscalls() | ||
| 87 | + var got: Sigaction | ||
| 88 | + check sigactionOf(SIGALRM, got) | ||
| 89 | + check got.sa_handler == handler | ||
| 90 | + check (got.sa_flags and p.SA_RESTART) != 0 | ||