diff --git a/ChangeLog.md b/ChangeLog.md --- a/ChangeLog.md +++ b/ChangeLog.md @@ -1,568 +1,164 @@ # ChangeLog -## 0.3.15 +## 0.3.16 -A server could not be stopped without closing a socket under it, and its -peers were told nothing when it was. +* Stop waiting for a socket to be readable, and run on Windows. + [#161](https://github.com/kazu-yamamoto/quic/pull/161) +* Count what arrives after the application has closed a stream. + [#163](https://github.com/kazu-yamamoto/quic/pull/163) +* Say that Windows needs the native I/O manager. + [#164](https://github.com/kazu-yamamoto/quic/pull/164) +* Stop forking a thread for every datagram a server receives. + [#165](https://github.com/kazu-yamamoto/quic/pull/165) -* Hand the caller the action that stops the server, through - `scInstallShutdownHandler`, the way warp hands out the action that - stops a warp. `stop` reaches a server through one of its connections, - and a server is at its emptiest when someone wants it to stop: one that - never took a connection, or has finished the ones it had, could not be - stopped at all. What a caller was left with was closing the socket out - from under the dispatcher waiting on it, which ends the server by - making it fail and closes a socket that in `runWithSockets` is not the - server's to close. The dispatchers already wait for a datagram and for - this at once, so they see it where they wait and end there: no socket - is closed and nothing is raised. Which matters beyond being tidy -- - waking a thread out of a wait by closing the file descriptor under it - is what `closeFdWith` is for, and the IO manager that provides it is - not the only one there will be. - [#159](https://github.com/kazu-yamamoto/quic/pull/159) +## 0.3.15 -* Tell the peers when the server stops. A server that stopped closed its - sockets and that was all they ever learned of it: each connection went - quiet and stayed quiet until the peer's idle timeout expired, half a - minute later, on a connection that was never going to answer again. - The connections are now ended through the same path that ends a - connection for any other reason, while the sockets are still open, so - each peer is sent a CONNECTION_CLOSE and can open a new connection at - once. It is an application close and `scCloseReason` says which: a - transport CONNECTION_CLOSE carrying NO_ERROR is what a connection whose - application has finished normally sends, and a client makes an - exception of it in a 1-RTT packet so that a server finishing is not an - error. A server that is going away is saying something else, and the - application protocol is where it is said -- HTTP/3 has H3_NO_ERROR for - it. +* Add `scInstallShutdownHandler` to stop a server. [#159](https://github.com/kazu-yamamoto/quic/pull/159) - -* Send the first CONNECTION_CLOSE before leaving the connection. The - frame was encoded where the connection ends and the sending left to a - closer thread that nothing waited for, so the connection ended before - it had gone anywhere -- and a server that stops lets go of its sockets - as soon as its connections have ended, so the send then failed on a - closed socket and the peer heard nothing at all. +* Tell the peers when the server stops. [#159](https://github.com/kazu-yamamoto/quic/pull/159) - -* Set `SO_REUSEPORT` on macOS and the BSDs. Replacing a server means - starting its successor while the old process still has the UDP port, - and `SO_REUSEADDR` alone does not allow that there: the second bind is - refused with "Address already in use" and the successor cannot start. - It is not set on Linux, where it would load balance one server's - datagrams across the two processes by a hash of the four-tuple. +* Send the first CONNECTION_CLOSE before leaving the connection. [#159](https://github.com/kazu-yamamoto/quic/pull/159) +* Set `SO_REUSEPORT` on macOS and the BSDs. + [#159](https://github.com/kazu-yamamoto/quic/pull/159) ## 0.3.14 -A buffer overrun on many streams at once, 0-RTT sent against no limit at -all, and two frames a receiver was taking on trust. - * Count the frame headers when packing many streams into one packet. - `sendStreamSmall` fills a packet from the send queue up to 1040 octets - and was counting only the stream data, but every stream whose turn - comes needs a STREAM frame of its own and nineteen octets of header - with it. Hundreds of streams writing a byte or two each therefore - built a packet far larger than the buffer it is encoded into, and the - sender died of `BufferOverrun`. Seen in Cloud Haskell, where one - connection carries hundreds of processes. [#153](https://github.com/kazu-yamamoto/quic/pull/153) - -* Hold a client sending 0-RTT to the limits the previous connection gave - it. RFC 9000 Sec 7.4.1 holds it to those until the server's own - arrive. `sendStreamMany` had a second road for 0-RTT that put the data - straight on the queue and told the flow control window about it - afterwards, so a resuming client could spend a connection window it had - not been given -- and a server that counts answers that with - FLOW_CONTROL_ERROR before the handshake has finished. Both roads go - through the check now, and the connection's own send limit is seeded - from the remembered parameters so that 0-RTT still carries data. It is - seeded beside the two stream counts that were already seeded there and - were being taken from `defaultParameters` -- 64 streams and ten - unidirectional, where a server may have allowed fewer, and since these - limits only ever rise one set too high stayed too high for the rest of - the connection. +* Hold a client sending 0-RTT to the limits of the previous connection. [#156](https://github.com/kazu-yamamoto/quic/pull/156) - -* Refuse a NEW_CONNECTION_ID that contradicts an earlier one. RFC 9000 - Sec 19.15 leaves to the receiver a connection ID repeated with a - different stateless reset token or a different sequence number, and a - sequence number used for a different connection ID. Ours looked the - connection ID up, found it, and took the frame for a retransmission - without comparing what had come with it. A retransmission says exactly - what it said before, so it still passes. +* Refuse a NEW_CONNECTION_ID that contradicts an earlier one. [#157](https://github.com/kazu-yamamoto/quic/pull/157) - -* Refuse a RETIRE_CONNECTION_ID for the connection ID the packet carrying - it arrived on, which RFC 9000 Sec 19.16 also leaves to the receiver. - An endpoint has to stop using a connection ID before retiring it, so - the packet that retires one is addressed to another and nothing correct - is caught by this. +* Refuse a RETIRE_CONNECTION_ID for the connection ID it arrived on. [#158](https://github.com/kazu-yamamoto/quic/pull/158) ## 0.3.13 -One line of debug output that could end the connection it described, and -three ways a failure or a coder left something behind. - -* A debug write can no longer end the connection it describes. With a - debug directory set every line goes to the connection's file and also - to stdout, and a daemon has no stdout to write to: it is closed, or a - pipe whose reader has gone. The write then throws in whichever - protocol thread happened to log, and six of those run under nested - `concurrently_` and take the rest down with them -- so the connection - ended over a line of debug output. The first write of all is the - original CID, before the connection has been built, so the peer heard - nothing at all and saw a handshake that never finished; a later one - arrived as an INTERNAL_ERROR, once 0.3.12 began saying when a - connection ends of something in here. Against mighty that was every - shape we had been chasing at once: the freeze, the CONNECTION_CLOSE - that never came, and the bursts of connections that died together. It - came and went because a stdout that is not a terminal is - block-buffered and it is the flush that fails. The writes now drop an - `IOException` -- only that, an asynchronous exception is not one, so - cancelling a thread that is logging still cancels it. +* A debug write can no longer end the connection it describes. [#148](https://github.com/kazu-yamamoto/quic/pull/148) - -* The qlog writer goes the same way and for the same reason. It is - called from the sender, the receiver and the closer, the same threads, - and the disk a qlog directory sits on can fill. +* A qlog write can no longer end the connection either. [#150](https://github.com/kazu-yamamoto/quic/pull/150) - -* Free what a failed setup took. A connection's setup, a client's, and - the server's own are each the acquire of their own `bracket`, and an - acquire that throws gets no release. A server connection was leaving - two log files, three 2048-byte buffers and a registration in the - dispatcher; a client the same, less a log file and plus its socket, - which nothing else closes because on the ordinary path the closer does - and a connection that never began has no closer; the server itself - every address it had already bound, the dispatchers on them and the - token manager thread. `closure''`, which runs on the way out of every - connection, left its buffers behind if the CONNECTION_CLOSE could not - be encoded. Setup does fail -- a client and a server in one process - pointed at one qlog directory ask for the same file, and the second is - told the file is busy -- and the bursts above were leaking two handles - apiece. +* Free what a failed setup took. [#149](https://github.com/kazu-yamamoto/quic/pull/149) - * The header protection mask no longer leaks a buffer for every coder. - It was taken with `mallocBytes` and freed nowhere: 16 bytes under the - AES-GCM ciphers and 32 under ChaCha20-Poly1305, four coders to a - connection at each end, held for the life of the process. A server - taking a thousand connections a second lost about 5GB a day. The - buffer was raw because `getMask` handed it out and the caller read it - after the call had returned, which nothing a garbage collector owns - would survive; the reader is handed in instead now, so the buffer can - be a `ForeignPtr` held across exactly the use. This changes - `Protector` in `Network.QUIC.Internal`. [#151](https://github.com/kazu-yamamoto/quic/pull/151) ## 0.3.12 -A server that looked frozen, a Stateless Reset half the peers threw away, -two windows that leaked, and the AEAD limits. - -* Tell the peer before waiting for the application. The protocol threads - and the application run under one `concurrently_`; when the protocol - threads failed it cancelled the application and then waited for it, - under `uninterruptibleMask_`, for as long as it took to unwind, and the - CONNECTION_CLOSE waited with it. A peer told nothing waits out its own - idle timeout, so from the outside the server had frozen at the - handshake. Seen against mighty, where a transport error the server - raised correctly never reached the client at all; it is said between - the two now, which is the one place it can be said. +* Tell the peer before waiting for the application. [#141](https://github.com/kazu-yamamoto/quic/pull/141) - -* Set the bit RFC 9000 fixes in a Stateless Reset. Sec 10.3 fixes the - first two bits at 01; the first byte was a random seven bits, so the - QUIC Bit was clear in half of them. A peer that has not asked for that - bit to be greased (RFC 9287) drops such a packet before anything looks - for a token in it -- and a Stateless Reset answers a packet we have no - connection for, so whether the peer asked is exactly what we cannot - know. Half went unheard, and the peer went on talking to a connection - that was gone until its idle timeout. +* Set the QUIC Bit in a Stateless Reset. [#137](https://github.com/kazu-yamamoto/quic/pull/137) - -* Tell the peer when a connection ends of something in here. `closure` - turned four exceptions into a CONNECTION_CLOSE and rethrew the rest, so - a connection that ended of anything else -- the application throwing, - StreamIsClosed, an IOException -- ended in silence. Those now end with - an INTERNAL_ERROR. An idle timeout, a peer that has already closed, - and an asynchronous exception stay silent, as RFC 9000 Sec 10.1 and - 10.2 have them. +* Send INTERNAL_ERROR when a connection ends of an unexpected exception. [#140](https://github.com/kazu-yamamoto/quic/pull/140) - * Count what the application never reads against the connection's window. - It moved in `recvStream` and nowhere else, so octets an application - left behind were counted as received and never as consumed, and the - window we advertise stayed that much smaller for the rest of the - connection. A server answering a request without reading its body is - the ordinary case, and it paid for that body until the connection - ended. [#142](https://github.com/kazu-yamamoto/quic/pull/142) - -* The AEAD limits of RFC 9001 Sec 6.6, neither of which was kept. - AEAD_LIMIT_REACHED was in the error table and nothing raised it. - Packets that fail authentication are counted now, across all keys, and - the connection closes past the integrity limit -- 2^52 for the AES-GCM - ciphers, 2^36 for ChaCha20-Poly1305. Packets each key protects are - counted too, and the connection closes past the confidentiality limit, - 2^23 under the AES-GCM ciphers. **Starting a key update instead of - closing is the better answer to the second and is not here**: the key - state holds one phase and the packet number the peer changed it at, - which is what the responding side needs and not what an initiating one - does. - [#145](https://github.com/kazu-yamamoto/quic/pull/145), +* Keep the AEAD limits of RFC 9001 Sec 6.6. + [#145](https://github.com/kazu-yamamoto/quic/pull/145) [#147](https://github.com/kazu-yamamoto/quic/pull/147) - -* Refuse a RETIRE_CONNECTION_ID we never issued. RFC 9000 Sec 19.16 - makes a sequence number greater than any we have sent a - PROTOCOL_VIOLATION; ours looked it up, found nothing and went on. One - we did send and have already retired stays ignored, since the frame may - simply have been sent twice. +* Refuse a RETIRE_CONNECTION_ID we never issued. [#144](https://github.com/kazu-yamamoto/quic/pull/144) - -* Say which thread ended a server connection. Six run under nested - `concurrently_`, which cancels the rest as soon as one fails; only the - sender and the receiver said why they ended, so a failure in one of the - other four left a log of two cancelled threads and no reason anywhere. - That is what made the frozen server above so hard to find. The other - half of it -- `runServer` discarding every message handed to its - `debugLog` -- went out in 0.3.11 unmentioned. - [#138](https://github.com/kazu-yamamoto/quic/pull/138), +* Say which thread ended a server connection. + [#138](https://github.com/kazu-yamamoto/quic/pull/138) [#139](https://github.com/kazu-yamamoto/quic/pull/139) ## 0.3.11 -A security fix, two things RFC 9000 asks of a receiver that were not -there, and a default that left a peer no room. - * Bind an address validation token to the address it was issued to. - RFC 9000 Sec 8.1.3: tokens sent in NEW_TOKEN frames MUST carry - something the server can check the client's address against, and if - the address has changed the server MUST keep to the anti-amplification - limit. Ours carried a version, a lifetime and, for a Retry, the - connection IDs -- no address -- and a fresh one was taken as proof, so - a client need only keep the NEW_TOKEN it was given and send it back - with someone else's address in the header: the server treated that - address as validated and answered it, certificate and all, having had - nothing proved to it. A token from before this cannot be decoded and - is already treated as no token at all, which is to say as an address - that has proved nothing. [#133](https://github.com/kazu-yamamoto/quic/pull/133) - -* Hold a peer to the final size it gave for a stream. FINAL_SIZE_ERROR - was in the error table and was never sent: where a stream ends, once - said, cannot be said differently, and nothing may arrive past it - (RFC 9000 Sec 4.5), but the final size a RESET_STREAM carries went to - a hook and nowhere else. That section also asks a receiver to count - the final size in its connection-level flow controller, and ours - counted what arrived; a peer counts the final size, so every stream it - reset with data still in flight left the two further apart, and the - window we advertise fell behind what the peer believed it had spent -- - by the tail of every reset, until it had none left. HTTP/3 cancels - requests as a matter of course. +* Hold a peer to the final size it gave for a stream. [#136](https://github.com/kazu-yamamoto/quic/pull/136) - -* Open the stream a STREAM_DATA_BLOCKED arrives for. RFC 9000 Sec 3.2 - has the receiving part of a peer's stream created by the first STREAM, - STREAM_DATA_BLOCKED or RESET_STREAM frame for it; the last was done in - 0.3.10 and this is the other. Both blocked frames also refuse a - packet that may not carry them: Table 3 has them in 0-RTT and 1-RTT - only, and Sec 12.4 makes a frame in a packet that may not carry it a - PROTOCOL_VIOLATION. +* Open the stream a STREAM_DATA_BLOCKED arrives for. [#134](https://github.com/kazu-yamamoto/quic/pull/134) - -* **The default `initial_max_streams_uni` is 10, where it was 3.** Three - is what HTTP/3 needs and no more -- a control stream and the two QPACK - streams (RFC 9114 Sec 6.2) -- so a peer given three could open nothing - else: no push stream, no stream of a type from an extension, and none - of the reserved types it is meant to open now and then so that the - types stay extensible. A client on these defaults could never be - pushed to. Ten leaves room for those without leaving the peer - unbounded, since 0.3.9 counts what is open at once and a stream gives - its place back when it is closed. +* The default `initial_max_streams_uni` is 10, where it was 3. [#135](https://github.com/kazu-yamamoto/quic/pull/135) - -* Say why a server connection ended. `runServer` logged the reason to a - logger that discards what it is given, so a connection that ended of - anything `closure` does not turn into a CONNECTION_CLOSE went without - a word to the peer and without a word in the log -- the peer talking - on to a connection that is gone until the dispatcher, a second later, - answers it with a Stateless Reset. From the outside that looks like a - server that froze. +* Say why a server connection ended. [#138](https://github.com/kazu-yamamoto/quic/pull/138) ## 0.3.10 -Two for the server: one that answered on a stream it should never have -been given, and one that made a lost flight cost seconds. - * Hand the application a stream only once its first frame is read. - `openStream` created the stream and gave it to the application in one - step, before the frame that opened it had been looked over, so a first - STREAM frame past the flow control limit was answered by the - application in the moment before the connection was closed over it. - An HTTP/3 server sent a response on a stream the peer had never - opened, and the peer called that a STREAM_STATE_ERROR, as RFC 9000 Sec - 19.8 says to, before the FLOW_CONTROL_ERROR arrived; h3spec's "MUST - send FLOW_CONTROL_ERROR if a STREAM frame with a large offset is - received" failed for that reason. The stream now reaches the - application after the frame has been checked, so a frame that closes - the connection closes it with the application none the wiser. [#131](https://github.com/kazu-yamamoto/quic/pull/131) - -* Keep the Initial keys of the version the client addressed us in. A - server that settles on a compatible version answers in it and replaces - its Initial keys with the ones for that version (RFC 9368), but the - client hears of the choice only when the answer arrives and until then - retransmits in the version it started with. Without the keys for that - version the server could not pick the handshake up from those and - waited out its own PTO instead -- a second, then two, then four, with - the client's retransmissions falling into it unread. On the test that - loses the server's whole first flight, the handshake is done at about - 1.4 seconds where it took about 7.0. This is the other half of #128, - which stopped the same sequence from hanging for good; what was left - was the waiting. +* Keep the Initial keys of the version the client addressed us in. [#132](https://github.com/kazu-yamamoto/quic/pull/132) - -* The library builds without a warning: two imports left over from the - crypton 2.1.0 work are gone. +* Build without warnings. ## 0.3.9 -A stream limit the peer could walk past, two ways for a connection to -hang, and the stream states. - -* Keep the peer's open streams within `initial_max_streams`. MAX_STREAMS - counts streams cumulatively (RFC 9000 Sec 4.6), so the limit should go - up by one for each of the peer's streams we are done with. It went up - by the highest stream the peer had opened plus the initial number, - every time one closed and the peer was near its limit -- a whole new - window for one closed stream. A peer that keeps its streams open could - have any number open at once: against 0.3.7 with a limit of 64, a - client whose server closed one stream in ten held 3132 open after three - seconds, and on an HTTP/3 server each of those is a handler thread. - The limit for unidirectional streams was taken from - `initialMaxStreamsBidi` as well, and `closeStream` on a unidirectional - stream the peer opened sent a FIN on a stream with no sending side, so - the peer closed the connection with STREAM_STATE_ERROR and those - streams could not be closed at all. +* Keep the peer's open streams within `initial_max_streams`. [#124](https://github.com/kazu-yamamoto/quic/pull/124) - -* Count what cannot be read against the anti-amplification limit. RFC - 9000 Sec 8.1 says to count all the payload bytes received, "including - datagrams that contain packets that are discarded"; they were counted - only for a packet that decrypted. A server that stops being able to - read the peer therefore stops earning the credit it needs to answer, - and its sender waits for good. Reached by way of compatible version - negotiation: the server answers in a version of its own and replaces - its Initial keys, its first flight is lost, the client retransmits in - the version it started with, and the server -- having sent exactly - three times what it read -- can neither read those nor send again. The - handshake never finishes. +* Count what cannot be read against the anti-amplification limit. [#128](https://github.com/kazu-yamamoto/quic/pull/128) - -* Open the stream a RESET_STREAM arrives for. RFC 9000 Sec 3.2 has the - receiving part of a peer's stream created by the first STREAM, - STREAM_DATA_BLOCKED or RESET_STREAM frame for it; only a STREAM frame - created it, and a RESET_STREAM that found none was dropped. It arrives - first whenever the peer resets a stream it has just sent on, since the - sender empties the queue the RESET_STREAM is on before the one the data - is on -- not a race but the order it works in. The stream was then - opened by the data that came after, with nothing to say it had been - reset and no FIN to end it, and `recvStream` waited on it until the - idle timeout. A stream never created is never counted, so each one - lost this way took a unit of MAX_STREAMS credit with it for good. +* Open the stream a RESET_STREAM arrives for. [#129](https://github.com/kazu-yamamoto/quic/pull/129) - -* Set the sending part closed on STOP_SENDING, not on RESET_STREAM. The - two end opposite directions and were the wrong way round. A - RESET_STREAM from the peer closed our sending part as well as the - receiving one, so a reply to what the peer sent before the reset could - not go out; a STOP_SENDING left our sending part open, so `sendStream` - went on working after we had answered with RESET_STREAM. **This - changes what callers see**: `sendStream` on a stream the peer stopped - now raises `StreamIsClosed`, where it used to succeed. `closeStream` - and `resetStream` also end the stream for its reader whatever else they - do -- with the sending part already closed they skipped it, and - `recvStream` blocked for good on a stream that had left the table and - could receive nothing more. And a MAX_STREAMS that does not raise the - limit is ignored (RFC 9000 Sec 4.6); one reordered or retransmitted - after a newer one lowered the limit on the streams we may open. +* Set the sending part closed on STOP_SENDING, not on RESET_STREAM. [#125](https://github.com/kazu-yamamoto/quic/pull/125) - -* Tests only: the IOSpec ports moved out of the range the kernel hands - out for a socket bound to port 0, which is 49152 to 65535 on macOS and - 32768 to 60999 on Linux. Inside it, the relay's own socket was now and - then given the very port the test server wanted, and "server never - became ready" followed -- about one run in three hundred. Each test - also waits for its server to stop before the next one starts; - `killThread` returns once the exception is delivered, not once the - thread is done with it, so the server outlived it and went on serving. - [#126](https://github.com/kazu-yamamoto/quic/pull/126), +* Tests only. + [#126](https://github.com/kazu-yamamoto/quic/pull/126) [#127](https://github.com/kazu-yamamoto/quic/pull/127) ## 0.3.8 -* Tell a stream that was reset from one that ended. After a - RESET_STREAM, `recvStream` returns an empty ByteString, just as it does - at the end of a stream, and nothing else said which it was. An - application protocol may have to know: HTTP/3's QPACK decoder has to - send a Stream Cancellation for a request stream that was reset - (RFC 9204 Sec 4.4.2), and a reset that lands between two frames looked - to it exactly like the end of the request. `resetReceived` answers the - error code of the peer's RESET_STREAM, or `Nothing` if there was none. +* Tell a stream that was reset from one that ended. [#123](https://github.com/kazu-yamamoto/quic/pull/123) ## 0.3.7 -* Don't open a closed stream again for a late copy of its data. A STREAM - frame for a stream no longer in the table opened it anew, and after the - stream was closed what arrives is a copy of data already received, sent - again because the packet carrying it was taken for lost while it was - only late. The new stream starts from the initial window, 256K, so a - copy from past that point was called a flow control error and the - connection closed with FLOW_CONTROL_ERROR; a copy from within the - window was worse, handing the application a closed stream as a new one. +* Don't open a closed stream again for a late copy of its data. [#118](https://github.com/kazu-yamamoto/quic/pull/118) - -* Report a server that could not be started. `run` and `runWithSockets` - created their sockets inside a handler whose logger discards what it is - given, so a failure to bind was swallowed, `onServerReady` was never - reached, and `run` returned as if all were well. **This changes what - callers see**: a `run` that cannot bind now raises where it used to - return quietly. +* Report a server that could not be started. [#119](https://github.com/kazu-yamamoto/quic/pull/119) - -* Don't let one datagram take the server's dispatcher down for good. An - exception anywhere in the dispatcher loop ended it, and nothing - restarts it -- the socket stays bound, so the server went on looking - like a server and answering nothing, with the failure logged to the - same discarding logger. Decode and dispatch are guarded per datagram - now. No input was found that raises; this is the blast radius being - closed, not a known hole. +* Don't let one datagram take the server's dispatcher down for good. [#120](https://github.com/kazu-yamamoto/quic/pull/120) - -* Tests only: the IOSpec relay no longer connects its sockets, since a - connected UDP socket turns an ICMP port-unreachable into ECONNREFUSED - on the next operation and the peers there come and go with every test. - And qlog is behind a `qlog` flag, off by default, which makes its own - directory when it is on -- the directory used to be the CI's job, so a - fresh clone failed 33 examples with `openFile: does not exist`. - [#117](https://github.com/kazu-yamamoto/quic/pull/117), +* Tests only. + [#117](https://github.com/kazu-yamamoto/quic/pull/117) [#121](https://github.com/kazu-yamamoto/quic/pull/121) ## 0.3.6 -A security fix and three for a stalled handshake. - -* Stop treating a token the server cannot decrypt as a validated address. - The dispatcher's wildcard caught the decryption failing and passed - `addrValid = True` on, which turns off the three-times - anti-amplification limit -- so a peer was better off sending rubbish in - the Token field than sending nothing, from any source address, with no - keys and no handshake. RFC 9000 Sec 8.1.3 says to proceed as if the - address were not validated. A token we did issue in NEW_TOKEN now has - its lifetime honoured as well; only the Retry path was checking expiry. +* Stop treating an undecryptable token as a validated address. [#114](https://github.com/kazu-yamamoto/quic/pull/114) -* Let the PTO probe reach the retransmission that is waiting for the - window. A client that sends 1-RTT before the handshake is confirmed - can deadlock its own handshake: RFC 9001 Sec 5.7 stops the peer - processing those packets, so they are never acknowledged and never - leave the congestion window, and the window cannot open until the - CRYPTO frame the peer is waiting for arrives. The probe may be sent - past a full window and was being spent on a bare PING, because a packet - already declared lost has left the sent-packet database and - `releaseOldest` cannot see it. An ACK-only packet no longer waits for - the window either (RFC 9002 Sec 7), which was blocking the one sender - thread and everything queued behind it. +* Let the PTO probe reach a retransmission waiting for the window. [#115](https://github.com/kazu-yamamoto/quic/pull/115) -* Spend the PTO probe on what is being held back rather than on a PING, - when the sender is already holding an ack-eliciting packet at the level - the timer fired for. +* Spend the PTO probe on data being held back rather than on a PING. [#113](https://github.com/kazu-yamamoto/quic/pull/113) -* Count only what is in flight into bytes in flight. RFC 9002 Sec 2: - a packet is in flight when it is ack-eliciting or contains PADDING. - Every packet sent was counted, so an ACK-only packet spent congestion - window it had no business spending -- 177 of 207 such sends in a - measured run. The predicate was already in `Types.Frame`, unused. +* Count only what is in flight into bytes in flight. [#116](https://github.com/kazu-yamamoto/quic/pull/116) - -* AES-GCM goes through crypton's one-call interface, and the bundled picotls - `fusion` engine is gone with the 11,017 lines of C it came in. What that - engine was for is that crypton rebuilt the AES key schedule and the table - of multiples of H for every packet, and took the header protection mask in - a second call after the encryption; crypton 2.1 builds the first once per - key and hands back the mask from the same call as the ciphertext. Measured - on an Apple M4, a 100-byte packet goes from 2.25 to 0.16 microseconds and a - 1440-byte one from 2.50 to 0.45. Against `fusion` itself, measured in C on - an Intel Haswell where it runs at all, crypton is at 91 to 96 per cent of - it for the call this makes -- and in Haskell `fusion` needs three foreign - calls to crypton's one. - -* ChaCha20-Poly1305 is in `defaultCiphers` on x86-64 again. It had been left - out there because `fusion` did not implement it, so a build with the engine - offered two suites where every other build offered three. The RFC 9001 and - RFC 9369 test vectors for it, skipped under the same condition, now run - everywhere. +* Use crypton's one-call AES-GCM interface and remove the bundled picotls + `fusion` engine and its cabal flag. +* ChaCha20-Poly1305 is in `defaultCiphers` on x86-64 again. [#111](https://github.com/kazu-yamamoto/quic/pull/111) - -* The `fusion` cabal flag is gone with the engine. A build passing - `-f fusion` will now fail on an unknown flag rather than quietly - selecting something that no longer exists. - -* The IOSpec relay no longer latches onto a leftover datagram at the port - handover, and qlog is kept for a failed CI job. Tests and CI only, but - the qlog is what made the stalls above findable at all. +* Tests and CI: fix the IOSpec relay and keep qlog for a failed job. [#112](https://github.com/kazu-yamamoto/quic/pull/112) ## 0.3.5 -Security fixes. The first four can be reached by a peer that has not -authenticated itself. - -* Drop a packet whose header protection sample is not whole. A sample of 1 - to 15 octets reached the cipher, which raised rather than answering with a - short mask, and the connection went with it. One conforming datagram did - it. +* Drop a packet whose header protection sample is not whole. [#95](https://github.com/kazu-yamamoto/quic/pull/95) -* Bound the CRYPTO data held out of order. CRYPTO frames sit outside the - flow control that bounds stream data, so nothing stopped a peer parking - fragments at scattered offsets and having every one held. - CryptoBufferExceeded had been defined and never used. +* Bound the CRYPTO data held out of order. [#96](https://github.com/kazu-yamamoto/quic/pull/96) -* Decode the peer's transport parameters to the Maybe the type promises, - rather than raising BufferOverrun out of a pure value. +* Decode the peer's transport parameters without raising BufferOverrun. [#97](https://github.com/kazu-yamamoto/quic/pull/97) * Refuse a transport parameter sent twice, and stream limits past 2^60. [#105](https://github.com/kazu-yamamoto/quic/pull/105) * Stop the sender deadlocking on a congestion window it cannot free. - Padding an ACK-only packet put it in flight, spending window that nothing - would give back once the loss timer had been cancelled, and the loss timer - could not be re-armed from another level. [#103](https://github.com/kazu-yamamoto/quic/pull/103) * Leave the peer a whole header protection sample when encoding. [#104](https://github.com/kazu-yamamoto/quic/pull/104) * Bound a connection id and a Retry packet in the long header decoder. [#106](https://github.com/kazu-yamamoto/quic/pull/106) -* Bound how many pieces a stream may be held in. Flow control counts octets, - not fragments, and a fragment costs far more than the octet it carries. +* Bound how many pieces a stream may be held in. [#108](https://github.com/kazu-yamamoto/quic/pull/108) -* Check the ranges an ACK frame carries, and refuse an ACK for a packet never +* Check the ranges of an ACK frame, and refuse an ACK for a packet never sent. [#109](https://github.com/kazu-yamamoto/quic/pull/109) -* Give the two ends of a connection their own qlog file. Pointing both at - one directory took the server down. +* Give the two ends of a connection their own qlog file. [#100](https://github.com/kazu-yamamoto/quic/pull/100) * Remove the partial functions that were worth removing. [#107](https://github.com/kazu-yamamoto/quic/pull/107) -* Requiring crypton v2.0.1, whose 2.0.0 dispatched an XOP instruction on - CPUs without XOP. - [crypton#202](https://github.com/kazu-yamamoto/crypton/issues/202) -* This is a patch release, but `Network.QUIC.Internal` changed: - `fromAckInfoWithMin` is gone, `FlowCntl` has `TooFragmented`, and - `tryReassemble` returns `FlowCntl` rather than `Bool`. +* Require crypton 2.0.1. +* `Network.QUIC.Internal` changed. ## 0.3.4 diff --git a/Network/QUIC.hs b/Network/QUIC.hs --- a/Network/QUIC.hs +++ b/Network/QUIC.hs @@ -3,6 +3,11 @@ -- | This main module provides APIs for QUIC. -- -- The -threaded option must be specified to GHC to use this library. +-- +-- On Windows the native I/O manager must be selected as well, with +-- @-with-rtsopts=--io-manager=native@ or @+RTS --io-manager=native@. +-- Under the other one (MIO) a handshake does not complete. A library +-- cannot choose this, so the application has to. module Network.QUIC ( -- * Connection Connection, diff --git a/Network/QUIC/Client/Reader.hs b/Network/QUIC/Client/Reader.hs --- a/Network/QUIC/Client/Reader.hs +++ b/Network/QUIC/Client/Reader.hs @@ -27,6 +27,7 @@ import Network.QUIC.Recovery import Network.QUIC.Socket import Network.QUIC.Types +import Network.QUIC.Windows -- | readerClient dies when the socket is closed. readerClient :: Socket -> Connection -> IO () @@ -35,10 +36,16 @@ wait connected <- getSockConnected conn peersa0 <- peerSockAddr <$> getPathInfo conn + -- The idle timeout below throws into this thread to end the wait, and + -- 'killReaders' does the same when the connection closes. Neither + -- reaches a thread blocked in a socket call on Windows, so the call -- + -- the call alone, not the loop around it -- goes to a thread of its own + -- there. Nothing is left racing for a datagram: the timeout closes the + -- socket and stops, and closing it is also what ends an abandoned call. let recv - | connected = NSB.recv s0 2048 + | connected = windowsThreadBlockHack $ NSB.recv s0 2048 | otherwise = do - (bs, peersa) <- NSB.recvFrom s0 2048 + (bs, peersa) <- windowsThreadBlockHack $ NSB.recvFrom s0 2048 if peersa /= peersa0 then recv else return bs loop recv where diff --git a/Network/QUIC/Closer.hs b/Network/QUIC/Closer.hs --- a/Network/QUIC/Closer.hs +++ b/Network/QUIC/Closer.hs @@ -18,6 +18,7 @@ import Network.QUIC.Recovery import Network.QUIC.Sender import Network.QUIC.Types +import Network.QUIC.Windows closure :: Connection -> LDCC -> Either E.SomeException a -> IO a closure conn ldcc (Right x) = do @@ -100,10 +101,21 @@ let (recv, clos) = case mrecvbuf of Nothing -> (void $ connRecv conn, return ()) Just recvbuf -> + -- 'closer' puts a timeout around this, and a timeout + -- does not reach a thread blocked in a socket call on + -- Windows, so the call goes to a thread of its own + -- there. Each timeout abandons one; there are six at + -- most, they share a buffer nothing reads, and 'clos' + -- below closes the socket and ends them all. let recv' - | connected = void $ NS.recvBuf sock recvbuf bufsiz + | connected = + windowsThreadBlockHack $ + void $ + NS.recvBuf sock recvbuf bufsiz | otherwise = do - (_, sa) <- NS.recvBufFrom sock recvbuf bufsiz + (_, sa) <- + windowsThreadBlockHack $ + NS.recvBufFrom sock recvbuf bufsiz when (sa /= peersa) recv' clos' = do NS.close sock diff --git a/Network/QUIC/Connection/StreamTable.hs b/Network/QUIC/Connection/StreamTable.hs --- a/Network/QUIC/Connection/StreamTable.hs +++ b/Network/QUIC/Connection/StreamTable.hs @@ -6,12 +6,15 @@ findStream, addStream, delStream, + keepDepartedStream, + noteDepartedRxFrame, initialRxMaxStreamData, setupCryptoStreams, clearCryptoStream, getCryptoStream, ) where +import qualified Data.IntMap.Strict as IntMap import qualified Data.IntSet as IntSet import Network.QUIC.Connection.Misc @@ -74,6 +77,68 @@ delStream :: Connection -> Stream -> IO () delStream Connection{..} strm = atomicModifyIORef'' streamTable $ deleteStream $ streamId strm + +---------------------------------------------------------------- + +-- | Keeping account of a stream the application has closed before the peer +-- finished it. +-- +-- A frame that arrives for a stream no longer in the table is dropped, and +-- what the peer spent on it is then counted by nobody: not as received, +-- since we never looked at it, and not as consumed, since nobody will read +-- it. The window we advertise falls that much behind what the peer +-- believes it has spent, for the rest of the connection, and a server that +-- answers requests without reading their bodies runs its peers out of +-- window. 'Network.QUIC.IO.releaseStream' gives back what had arrived by +-- the time of the close; this is for what arrives after it. +-- +-- Nothing is kept for a stream the peer has finished: its final size is +-- known and accounted for, and anything that still arrives for it lies +-- inside that and has been counted already. +keepDepartedStream :: Connection -> Stream -> IO () +keepDepartedStream Connection{..} strm = do + b <- getRxBounds strm + unless (settled b) $ + atomicModifyIORef'' departedStreams $ + IntMap.insert (streamId strm) b + +-- | Is there nothing more this stream can owe? +settled :: RxBounds -> Bool +settled RxBounds{..} = rxFinal == Just rxCounted + +-- | Noting a frame that arrived for a stream the application has closed, +-- and answering with what the connection's window is owed for it. +-- +-- The peer's own flow control counts the offsets it has sent, not the +-- octets that reached us, so what is owed is measured the same way: the +-- furthest point of the stream anything has reached, less what has been +-- accounted for already. A retransmission does not move that point and so +-- is owed nothing, which is what keeps this from counting twice -- the +-- reassembly that tells a copy from new data went with the stream. +noteDepartedRxFrame :: Connection -> StreamId -> Int -> Bool -> IO Int +noteDepartedRxFrame Connection{..} sid end fin = + atomicModifyIORef' departedStreams note + where + note tbl = case IntMap.lookup sid tbl of + Nothing -> (tbl, 0) + Just b -> + let b' = account b + in ( if settled b' then IntMap.delete sid tbl else IntMap.insert sid b' tbl + , rxCounted b' - rxCounted b + ) + account b@RxBounds{..} = + b + { rxCounted = reached + , rxHighest = max rxHighest end + , rxFinal = if fin then Just end else rxFinal + } + where + -- Never past the end the peer has said the stream has: a frame + -- claiming to reach beyond it is for the live path to refuse, and + -- this one is not the place to answer it. + reached = case (if fin then Just end else rxFinal) of + Just f -> min f $ max rxCounted end + Nothing -> max rxCounted end initialRxMaxStreamData :: Connection -> StreamId -> Int initialRxMaxStreamData conn sid diff --git a/Network/QUIC/Connection/Types.hs b/Network/QUIC/Connection/Types.hs --- a/Network/QUIC/Connection/Types.hs +++ b/Network/QUIC/Connection/Types.hs @@ -289,6 +289,10 @@ , -- State peerPacketNumber :: IORef PacketNumber -- for RTT1 , streamTable :: IORef StreamTable + , -- | What a stream the application has closed still owes the + -- connection's window, for as long as the peer has not finished it. + -- See 'Network.QUIC.Connection.StreamTable.keepDepartedStream'. + departedStreams :: IORef (IntMap RxBounds) , myStreamId :: TVar Concurrency -- C:0 S:1 , myUniStreamId :: TVar Concurrency -- C:2 S:3 , peerStreamId :: IORef Concurrency -- C:1 S:0 @@ -398,6 +402,7 @@ -- State peerPacketNumber <- newIORef 0 streamTable <- newIORef emptyStreamTable + departedStreams <- newIORef IntMap.empty myStreamId <- newTVarIO (newConcurrency rl Bidirectional 0) myUniStreamId <- newTVarIO (newConcurrency rl Unidirectional 0) peerStreamId <- newIORef peerConcurrency diff --git a/Network/QUIC/IO.hs b/Network/QUIC/IO.hs --- a/Network/QUIC/IO.hs +++ b/Network/QUIC/IO.hs @@ -199,12 +199,20 @@ -- that body until the connection ends, and enough of them leave the -- peer blocked by octets nobody is waiting for. unread <- takeRxUnread s - when (unread > 0) $ do - mx <- updateFlowRx conn unread + -- And, if the peer has said where the stream ends, the rest of it: + -- RFC 9000 Sec 4.5 has a receiver account for every octet sent on a + -- stream, and what was lost on the way was still spent. + uncounted <- takeRxUncounted s + let owed = unread + uncounted + when (owed > 0) $ do + mx <- updateFlowRx conn owed forM_ mx $ \newMax -> do sendFrames conn RTT1Level [MaxData newMax] fire conn (Microseconds 50000) $ sendFrames conn RTT1Level [MaxData newMax] + -- Where the peer has not said, what arrives from here on is still + -- owed and the stream is gone, so what it owes is kept without it. + keepDepartedStream conn s where conn = streamConnection s sid = streamId s diff --git a/Network/QUIC/Receiver.hs b/Network/QUIC/Receiver.hs --- a/Network/QUIC/Receiver.hs +++ b/Network/QUIC/Receiver.hs @@ -321,6 +321,17 @@ where retired = [n | RetireConnectionID n <- frames] +-- | Giving the connection's window back what a frame for a stream the +-- application has closed is owed. +creditDeparted :: Connection -> StreamId -> Int -> Bool -> IO () +creditDeparted conn sid end fin = do + owed <- noteDepartedRxFrame conn sid end fin + when (owed > 0) $ do + mx <- updateFlowRx conn owed + forM_ mx $ \newMax -> do + sendFrames conn RTT1Level [MaxData newMax] + fire conn (Microseconds 50000) $ sendFrames conn RTT1Level [MaxData newMax] + processFrame :: Connection -> EncryptionLevel -> Frame -> IO () processFrame _ _ Padding{} = return () processFrame conn lvl Ping = do @@ -371,7 +382,10 @@ mstrm <- maybe (openStream conn sid) (return . Just) mstrm0 onResetStreamReceived2 (connHooks conn) mstrm aerr finlen case mstrm of - Nothing -> return () + -- As for a STREAM frame arriving for a stream the application has + -- closed: this says where the stream ends, so what the peer spent on + -- it is known in full and owed in full. + Nothing -> creditDeparted conn sid finlen True Just strm -> do -- RFC 9000 Sec 4.5, as for a STREAM frame that ends the stream. noteRxFinalSize strm finlen >>= closeOverFinalSize conn @@ -508,33 +522,37 @@ -- acknowledgement crossed it. Opening the stream anew would hold -- the copy to the initial window and call it a flow control error. mstrm' <- maybe (openStream conn sid) (return . Just) mstrm - forM_ mstrm' $ \strm -> do - let len = BS.length dat - rx = RxStreamData dat off len fin - -- RFC 9000 Sec 4.5: where the stream ends, once said, cannot be said - -- differently, and nothing may arrive past it. - noteRxFrame strm (off + len) fin >>= closeOverFinalSize conn - fc <- putRxStreamData strm rx - case fc of - -- FLOW CONTROL: MAX_STREAM_DATA: recv: rejecting if over my limit - OverLimit -> - closeConnection conn FlowControlError "Flow control error for stream in 1-RTT" - -- Not a flow control error: the peer is inside its window, it is - -- just spending it in more pieces than we will hold. Rate control - -- answers with InternalError too. - TooFragmented -> - closeConnection conn QUIC.InternalError "Too many stream fragments" - Duplicated -> return () - Reassembled -> do - addRxCounted strm len - ok' <- checkRxMaxData conn len - -- FLOW CONTROL: MAX_DATA: send: respecting peer's limit - unless ok' $ - closeConnection - conn - FlowControlError - "Flow control error for connection in 1-RTT" - deliverStream conn mstrm strm + case mstrm' of + -- The stream is closed and the data goes nowhere, but the peer spent + -- its connection window on it and is owed that back. + Nothing -> creditDeparted conn sid (off + BS.length dat) fin + Just strm -> do + let len = BS.length dat + rx = RxStreamData dat off len fin + -- RFC 9000 Sec 4.5: where the stream ends, once said, cannot be + -- said differently, and nothing may arrive past it. + noteRxFrame strm (off + len) fin >>= closeOverFinalSize conn + fc <- putRxStreamData strm rx + case fc of + -- FLOW CONTROL: MAX_STREAM_DATA: recv: rejecting if over my limit + OverLimit -> + closeConnection conn FlowControlError "Flow control error for stream in 1-RTT" + -- Not a flow control error: the peer is inside its window, it + -- is just spending it in more pieces than we will hold. Rate + -- control answers with InternalError too. + TooFragmented -> + closeConnection conn QUIC.InternalError "Too many stream fragments" + Duplicated -> return () + Reassembled -> do + addRxCounted strm len + ok' <- checkRxMaxData conn len + -- FLOW CONTROL: MAX_DATA: send: respecting peer's limit + unless ok' $ + closeConnection + conn + FlowControlError + "Flow control error for connection in 1-RTT" + deliverStream conn mstrm strm processFrame conn lvl (MaxData n) = do when (lvl == InitialLevel || lvl == HandshakeLevel) $ closeConnection conn ProtocolViolation "MAX_DATA in Initial or Handshake" diff --git a/Network/QUIC/Server/Reader.hs b/Network/QUIC/Server/Reader.hs --- a/Network/QUIC/Server/Reader.hs +++ b/Network/QUIC/Server/Reader.hs @@ -11,6 +11,8 @@ waitNoConnection, tokenMgr, genStatelessReset, + stopServerNow, + wakeDispatcher, -- * Accepting Accept (..), @@ -23,8 +25,8 @@ import Control.Concurrent import Control.Concurrent.STM -import qualified Control.Monad.STM as STM import qualified Control.Exception as E +import qualified Control.Monad.STM as STM import qualified Crypto.Token as CT import qualified Data.ByteString as BS import Data.Map.Strict (Map) @@ -33,7 +35,13 @@ import Network.ByteOrder import Network.Control (LRUCacheRef, Rate, getRate, newRate) import qualified Network.Control as LRUCache -import Network.Socket (SockAddr, Socket, waitReadSocketSTM) +import Network.Socket ( + SockAddr (..), + Socket, + getSocketName, + tupleToHostAddress, + tupleToHostAddress6, + ) import qualified Network.Socket.ByteString as NSB import qualified System.IO.Error as E import System.Log.FastLogger @@ -49,7 +57,6 @@ import Network.QUIC.Parameters import Network.QUIC.Qlog import Network.QUIC.Types -import Network.QUIC.Windows ---------------------------------------------------------------- @@ -60,13 +67,16 @@ , genStatelessReset :: CID -> StatelessResetToken , statelessResetRate :: Rate , connectionCount :: TVar Int + , stopServerNow :: IO () + -- ^ Stopping this server: what the shutdown handler is handed, and + -- what a connection's 'stop' reaches it by. } statelessResetLimit :: Int statelessResetLimit = 20 -newDispatch :: ServerConfig -> IO Dispatch -newDispatch ServerConfig{..} = +newDispatch :: ServerConfig -> IO () -> IO Dispatch +newDispatch ServerConfig{..} stopNow = Dispatch <$> CT.spawnTokenManager conf <*> newIORef emptyConnectionDict @@ -74,6 +84,7 @@ <*> makeGenStatelessReset <*> newRate <*> newTVarIO 0 + <*> pure stopNow where conf = CT.defaultConfig @@ -174,21 +185,37 @@ data ServerState = Running | Stopped deriving (Eq, Show) -checkLoop :: TVar ServerState -> Socket -> IO Bool -checkLoop stvar mysock = do - st0 <- readTVarIO stvar - if st0 == Stopped - then return False - else do - wait <- waitReadSocketSTM mysock - atomically $ do - st <- readTVar stvar - if st == Stopped - then return False - else do - wait -- blocking is retry - return True +-- | Waking a dispatcher that is waiting for a datagram, by sending it one. +-- +-- The dispatchers used to wait for the socket to become readable and for the +-- stop variable at once, in a single 'atomically', and so saw a stop where +-- they waited. Readiness is not something every I/O manager has to report: +-- a completion-based one has nothing to say about a socket being readable, +-- and on Windows the wait reaches 'waitRead#', which the threaded RTS does +-- not merely refuse but aborts the process over. So the wait is a plain +-- receive now, and stopping has to put something into it. +-- +-- An empty datagram to the socket's own address does that, and keeps what +-- the shutdown handler promises: no socket is closed and nothing is raised. +-- The loop reads the stop variable again when the receive returns and ends +-- there. An empty datagram carries no packet, so one arriving from anywhere +-- else is just as harmless. +wakeDispatcher :: Socket -> IO () +wakeDispatcher s = E.handle ignore $ do + sa <- getSocketName s + void $ NSB.sendTo s "" $ reachable sa +-- | The address a socket can be reached on from the host it is bound on. +-- A wildcard bind is not an address to send to, so the loopback stands in +-- for it. +reachable :: SockAddr -> SockAddr +reachable (SockAddrInet p a) + | a == 0 = SockAddrInet p $ tupleToHostAddress (127, 0, 0, 1) +reachable (SockAddrInet6 p f a sc) + | a == (0, 0, 0, 0) = + SockAddrInet6 p f (tupleToHostAddress6 (0, 0, 0, 0, 0, 0, 0, 1)) sc +reachable sa = sa + dispatcher :: Dispatch -> ServerConfig @@ -200,9 +227,17 @@ labelMe "QUIC dispatcher" handleLogUnit logAction loop where + -- The loop used to wait for the socket to become readable, watching the + -- stop variable in the same 'atomically' so that 'stop' broke it without + -- an exception. Readiness is not a thing a completion-based I/O manager + -- reports, so on Windows that wait is not available at all; the loop + -- simply blocks in the receive instead, and 'stop' is answered by the + -- 'killThread' and the 'close' that 'run's own teardown does next. The + -- stop variable is still read, for the datagram that arrives between the + -- two. loop = do - cont <- checkLoop stvar mysock - when cont $ do + st <- readTVarIO stvar + when (st == Running) $ do (bs, peersa) <- safeRecv $ NSB.recvFrom mysock 2048 now <- getTimeMicrosecond let send' b = void $ NSB.sendTo mysock b peersa @@ -227,8 +262,13 @@ logAction _msg = return () + -- No thread of its own for the receive. The dispatcher is not ended by + -- an exception thrown into it -- it is told, and sees it where it waits + -- -- so it does not need to be interruptible there, and a forked thread + -- for every datagram a server receives is 4.5 microseconds of each on + -- Windows. safeRecv rcv = do - ex <- E.try $ windowsThreadBlockHack rcv + ex <- E.try rcv case ex of Right x -> return x Left se | isAsyncException se -> E.throwIO (se :: E.SomeException) diff --git a/Network/QUIC/Server/Run.hs b/Network/QUIC/Server/Run.hs --- a/Network/QUIC/Server/Run.hs +++ b/Network/QUIC/Server/Run.hs @@ -44,14 +44,19 @@ run conf server = do labelMe "QUIC run" stvar <- newTVarIO Running - installShutdownHandler conf stvar + -- The sockets do not exist yet and the stop has to reach them, so what + -- stops this server is built here and what it wakes is filled in by + -- 'setup'. See 'wakeDispatcher'. + wakeRef <- newIORef $ return () + let stopNow = stopping stvar wakeRef + installShutdownHandler conf stopNow -- Outside handleLogUnit on purpose. If the addresses cannot be bound -- there is no server, and that is the caller's business: swallowing it -- returned from 'run' as if all were well, having never reached -- onServerReady, and left anyone waiting on that hook waiting for good. -- An IOSpec run wedged for two and a half days that way, on a port the -- previous test had not finished releasing. - E.bracket (setup stvar) teardown $ \(_, _, _) -> handleLogUnit debugLog $ do + E.bracket (setup stvar stopNow wakeRef) (teardown stopNow) $ \(_, _, _) -> handleLogUnit debugLog $ do onServerReady $ scHooks conf atomically $ do st <- readTVar stvar @@ -63,20 +68,33 @@ -- port is taken leaves the first socket bound and the token manager -- thread running, for good. Each step frees what the steps before it -- took, which for a list means each element frees itself. - setup stvar = do - dispatch <- newDispatch conf + setup stvar stopNow wakeRef = do + dispatch <- newDispatch conf stopNow let forkConn acc = void $ forkIO $ withConnectionCount dispatch $ - runServer conf server dispatch stvar acc + runServer conf server dispatch acc flip E.onException (clearDispatch dispatch) $ do ssas <- openAll $ scAddresses conf + writeIORef wakeRef $ mapM_ wakeDispatcher ssas tids <- runAll dispatch conf stvar forkConn ssas `E.onException` mapM_ NS.close ssas return (dispatch, tids, ssas) - teardown (dispatch, tids, ssas) = do + -- Telling the dispatchers first. They end where they wait, on the + -- datagram 'stopping' sends them, and the 'killThread' below then has + -- nothing to reach -- which matters because on Windows it could not + -- have reached them anyway: a thread blocked in a socket call there is + -- not interruptible. Running them on a thread of their own so that it + -- could be was a forked thread for every datagram the server received. + -- + -- The usual way here is through the shutdown handler, which has already + -- done this; doing it again is free. The way that had not, until now, + -- is 'run' being cancelled from outside. + teardown :: IO () -> (Dispatch, [ThreadId], [NS.Socket]) -> IO () + teardown stopNow (dispatch, tids, ssas) = do + stopNow clearDispatch dispatch mapM_ killThread tids shutdownConnections conf dispatch @@ -92,9 +110,12 @@ runWithSockets ssas conf server = do labelMe "QUIC runWithSockets" stvar <- newTVarIO Running - installShutdownHandler conf stvar + -- As in 'run', except that the sockets are in hand already. + wakeRef <- newIORef $ mapM_ wakeDispatcher ssas + let stopNow = stopping stvar wakeRef + installShutdownHandler conf stopNow -- As in 'run'. - E.bracket (setup stvar) teardown $ \(_, _) -> handleLogUnit debugLog $ do + E.bracket (setup stvar stopNow) (teardown stopNow) $ \(_, _) -> handleLogUnit debugLog $ do onServerReady $ scHooks conf atomically $ do st <- readTVar stvar @@ -103,17 +124,20 @@ debugLog _msg = return () -- As in 'run'. The sockets are the caller's here, so only the token -- manager and the dispatchers are ours to free. - setup stvar = do - dispatch <- newDispatch conf + setup stvar stopNow = do + dispatch <- newDispatch conf stopNow let forkConn acc = void $ forkIO $ withConnectionCount dispatch $ - runServer conf server dispatch stvar acc + runServer conf server dispatch acc flip E.onException (clearDispatch dispatch) $ do tids <- runAll dispatch conf stvar forkConn ssas return (dispatch, tids) - teardown (dispatch, tids) = do + -- As in 'run'. + teardown :: IO () -> (Dispatch, [ThreadId]) -> IO () + teardown stopNow (dispatch, tids) = do + stopNow clearDispatch dispatch mapM_ killThread tids shutdownConnections conf dispatch @@ -131,10 +155,16 @@ -- out of a wait by closing the file descriptor under it is what -- 'closeFdWith' is for, and the IO manager that provides it is not the only -- one there will be. -installShutdownHandler :: ServerConfig -> TVar ServerState -> IO () -installShutdownHandler conf stvar = - scInstallShutdownHandler conf $ atomically $ writeTVar stvar Stopped +installShutdownHandler :: ServerConfig -> IO () -> IO () +installShutdownHandler = scInstallShutdownHandler +-- | Telling this server to stop: the state the dispatchers read, and then +-- the datagram that gets them to read it. +stopping :: TVar ServerState -> IORef (IO ()) -> IO () +stopping stvar wakeRef = do + atomically $ writeTVar stvar Stopped + join $ readIORef wakeRef + -- | Ending the connections the server still has, while the sockets are -- still open. -- @@ -192,10 +222,9 @@ :: ServerConfig -> (Connection -> IO ()) -> Dispatch - -> TVar ServerState -> Accept -> IO () -runServer conf server0 dispatch stvar acc = do +runServer conf server0 dispatch acc = do labelMe "QUIC runServer" E.bracket open clse $ \(ConnRes conn myAuthCIDs _reader) -> handleLogUnit (debugLog conn) $ do @@ -275,7 +304,7 @@ setConnectionClosed conn closure conn ldcc ex where - open = createServerConnection conf dispatch acc stvar + open = createServerConnection conf dispatch acc clse connRes = do let conn = connResConnection connRes setDead conn @@ -293,9 +322,8 @@ :: ServerConfig -> Dispatch -> Accept - -> TVar ServerState -> IO ConnRes -createServerConnection conf@ServerConfig{..} dispatch Accept{..} stvar = do +createServerConnection conf@ServerConfig{..} dispatch Accept{..} = do sref <- newIORef accMySocket pathInfo <- newPathInfo accPeerSockAddr piref <- newIORef $ PeerInfo pathInfo Nothing @@ -365,7 +393,7 @@ let mgr = tokenMgr dispatch setTokenManager conn mgr -- - setStopServer conn $ atomically $ writeTVar stvar Stopped + setStopServer conn $ stopServerNow dispatch -- setRegister conn accRegister accUnregister accRegister myCID conn diff --git a/Network/QUIC/Stream.hs b/Network/QUIC/Stream.hs --- a/Network/QUIC/Stream.hs +++ b/Network/QUIC/Stream.hs @@ -8,6 +8,7 @@ StreamState (..), RecvStreamQ (..), RxStreamData (..), + RxBounds (..), Length, syncFinTx, waitFinTx, @@ -24,6 +25,7 @@ setResetReceived, markReleased, FinalSizeProblem (..), + getRxBounds, noteRxFrame, noteRxFinalSize, addRxCounted, diff --git a/Network/QUIC/Stream/Misc.hs b/Network/QUIC/Stream/Misc.hs --- a/Network/QUIC/Stream/Misc.hs +++ b/Network/QUIC/Stream/Misc.hs @@ -13,6 +13,7 @@ setResetReceived, markReleased, FinalSizeProblem (..), + getRxBounds, noteRxFrame, noteRxFinalSize, addRxCounted, @@ -155,6 +156,10 @@ -- | Taking in where one STREAM frame says the stream reaches, and whether it -- ends it. Nothing is counted here: a frame may still turn out to be a -- duplicate, and only what is taken counts. +-- | What the receiving side has seen of where this stream ends. +getRxBounds :: Stream -> IO RxBounds +getRxBounds Stream{..} = readIORef streamRxBounds + noteRxFrame :: Stream -> Int -> Bool -> IO (Maybe FinalSizeProblem) noteRxFrame Stream{..} end fin = atomicModifyIORef' streamRxBounds note where diff --git a/Network/QUIC/Windows.hs b/Network/QUIC/Windows.hs --- a/Network/QUIC/Windows.hs +++ b/Network/QUIC/Windows.hs @@ -1,5 +1,6 @@ {-# LANGUAGE CPP #-} +-- | Making a blocking socket call interruptible on Windows. module Network.QUIC.Windows ( windowsThreadBlockHack, ) where @@ -7,15 +8,37 @@ #if defined(mingw32_HOST_OS) import Control.Concurrent import qualified Control.Exception as E -import Control.Monad +import GHC.Conc.Sync (labelThread) +-- | Running a blocking call on a thread of its own, and waiting for it +-- here. +-- +-- A thread blocked in a socket call on Windows cannot be reached by an +-- asynchronous exception: neither 'killThread' nor the timeouts quic builds +-- on the event manager get at it, and the readiness this package used to +-- wait for instead -- 'threadWaitReadSTM' and so the socket's STM wrappers +-- -- is not available under the native I/O manager at all, because +-- completion ports have nothing to say about a socket being readable. +-- +-- So the call goes to a thread of its own and this one waits on an MVar, +-- which is interruptible. It is the classic answer; warp has used it since +-- 3.2.17. +-- +-- The call is abandoned rather than cancelled. When this thread is +-- interrupted the forked one stays in the call until the socket is closed, +-- so this belongs only where the socket is closed soon after -- and where +-- another reader starting on the same socket in the meantime would do no +-- harm, since the two would then race for the next datagram. +-- +-- On every other platform the thread can be reached and this is 'id'. windowsThreadBlockHack :: IO a -> IO a windowsThreadBlockHack act = do var <- newEmptyMVar :: IO (MVar (Either E.SomeException a)) - void . forkIO $ E.try act >>= putMVar var + tid <- forkIO $ E.try act >>= putMVar var + labelThread tid "QUIC blocking call" res <- takeMVar var case res of - Left e -> print e >> E.throwIO e + Left e -> E.throwIO e Right r -> return r #else windowsThreadBlockHack :: IO a -> IO a diff --git a/quic.cabal b/quic.cabal --- a/quic.cabal +++ b/quic.cabal @@ -1,6 +1,6 @@ cabal-version: 2.0 name: quic -version: 0.3.15 +version: 0.3.16 license: BSD3 license-file: LICENSE maintainer: kazu@iij.ad.jp @@ -137,7 +137,7 @@ crypton-x509-store >=1.9.0 && <1.10, crypton-x509-system >=1.9.0 && <1.10, crypton-x509-validation >=1.9.0 && <1.10, - fast-logger >=3.2.2 && <3.3, + fast-logger >=3.2.8 && <3.3, filepath, iproute >=1.7.12 && <1.8, network >=3.2.3, @@ -263,7 +263,7 @@ crypto-token, crypton, directory, - fast-logger >=3.2.2 && <3.3, + fast-logger >=3.2.8 && <3.3, filepath, hspec, network >=3.2.2, diff --git a/test/Config.hs b/test/Config.hs --- a/test/Config.hs +++ b/test/Config.hs @@ -1,5 +1,6 @@ {-# LANGUAGE CPP #-} {-# LANGUAGE OverloadedStrings #-} +{-# LANGUAGE ScopedTypeVariables #-} module Config ( makeTestServerConfig, @@ -10,6 +11,7 @@ prepareQlog, setClientQlog, withPipe, + withTempDir, withPipeStray, Scenario (..), newSessionManager, @@ -27,9 +29,8 @@ import Network.Socket import Network.Socket.ByteString import Network.TLS hiding (Version) -#ifdef QLOG -import System.Directory (createDirectoryIfMissing) -#endif +import System.Directory +import System.FilePath (()) import Network.QUIC.Client import Network.QUIC.Internal @@ -161,14 +162,16 @@ withPipe :: Scenario -> IO () -> IO () withPipe = withPipeWith False --- | 'withPipe', with one short-header datagram delivered to the relay's --- socket before the relay starts reading. +-- | 'withPipe', with two leftover datagrams delivered to the relay's socket +-- before the relay starts reading: a short-header one and a long-header one. -- --- That is what the CONNECTION_CLOSE of the connection that just closed looks --- like when it lands after this socket has taken over the port, and taking it --- for the client ties the relay to a peer with nothing left to say. The test --- that uses this fails within the idle timeout if the relay ever goes back to --- latching onto the first datagram it sees. +-- That is what the connection that just closed leaves behind when it lands +-- after this socket has taken over the port. A CONNECTION_CLOSE is a +-- short-header packet once the handshake is confirmed and a long-header one +-- before that, and taking either for the client ties the relay to a peer +-- with nothing left to say. The test that uses this fails within the idle +-- timeout if the relay ever goes back to latching onto the first datagram it +-- sees, or onto the first long-header one. withPipeStray :: Scenario -> IO () -> IO () withPipeStray = withPipeWith True @@ -212,8 +215,9 @@ addrAny <- resolve "0" bind sockS $ addrAddress addrAny when stray $ - E.bracket (openSocket addrC) close $ \sock -> + E.bracket (openSocket addrC) close $ \sock -> do void $ sendTo sock (BS.pack [0x40, 1, 2, 3]) saC + void $ sendTo sock (BS.pack [0xc0, 1, 2, 3]) saC -- The relaying threads have to stop before the sockets close. -- Run at the end of body instead, the kills are skipped whenever @@ -231,7 +235,8 @@ -- from client tid0 <- forkIO $ do -- Wait for the client to introduce itself, and take the first - -- long-header packet rather than the first datagram. + -- datagram that could be its opening one rather than the first + -- datagram. -- -- These sockets use one fixed port, so the socket for this test -- binds it a fraction of a millisecond after the previous test @@ -243,15 +248,27 @@ -- relay: it sends Initial packets until the idle timeout and -- hears nothing, the server never sees the connection at all. -- - -- A client always opens with a long header; a leftover from an - -- established connection is a short one. That tells them apart. + -- A client opens with an Initial packet in a datagram padded to + -- at least 1200 bytes, which RFC 9000 Sec 14.1 requires of it so + -- that the path is known to carry one. Nothing a connection + -- leaves behind is that: a CONNECTION_CLOSE is a short-header + -- packet once the handshake is confirmed and a long-header one + -- before that, and either way it is a fraction of that size. + -- Long header alone does not tell them apart. + -- + -- This is a guard, not the cure. A connection that is still + -- running sends datagrams that are a client's opening one in + -- every respect, because that is what they are, and no reading + -- of a datagram can say which test it belongs to. A test that + -- leaves a client behind breaks the next one however this + -- chooses, so a test must not leave one behind. (bs, saO) <- waitForClientHello sockC writeIORef peerRef $ Just saO n0 <- atomicModifyIORef' irefC $ \x -> (x + 1, x) dropPacket0 <- shouldDrop scenario True n0 unless dropPacket0 $ void $ sendTo sockS bs saS forever $ do - (bs1, sa) <- recvFrom sockC 2048 + (bs1, sa) <- relayRecv sockC -- Only from the client we latched onto. The connect this -- replaces did that in the kernel; doing it here keeps -- leftovers from the connection that just closed from being @@ -266,7 +283,7 @@ sendTo sockS bs1 saS -- from server tid1 <- forkIO $ forever $ do - (bs, _) <- recvFrom sockS 2048 + (bs, _) <- relayRecv sockS n <- atomicModifyIORef' irefS $ \x -> (x + 1, x) dropPacket <- shouldDrop scenario False n let isCC = BS.length bs < 200 @@ -276,11 +293,18 @@ delayIf (shouldDelay scenario False n) $ void $ sendTo sockC bs sa return (tid0, tid1) stopRelay (tid0, tid1) = killThread tid0 >> killThread tid1 + -- 'stopRelay' kills these threads and the brackets close the sockets + -- straight after, and a thread blocked in a socket call on Windows is + -- not reachable by 'killThread' -- it would be left in the call and + -- would die of the close instead, from a thread nobody is watching. + relayRecv sock = windowsThreadBlockHack $ recvFrom sock 2048 waitForClientHello sockC = do - (bs, saO) <- recvFrom sockC 2048 - if not (BS.null bs) && BS.head bs .&. 0x80 /= 0 + (bs, saO) <- relayRecv sockC + if isClientHello bs then return (bs, saO) else waitForClientHello sockC + isClientHello bs = + BS.length bs >= 1200 && BS.head bs .&. 0x80 /= 0 hints = defaultHints { addrSocketType = Network.Socket.Datagram @@ -348,3 +372,31 @@ -- | How long 'DelayClientPacket' and 'DelayServerPacket' hold a datagram. delayTime :: Int delayTime = 100000 + +-- | A directory of our own for a spec to put files in, taken away +-- afterwards. +-- +-- Windows will not delete a file that is open, and will refuse for a while +-- after it has been closed as well -- a virus scanner reading what was just +-- written is enough, and that is the normal state of a CI runner. So the +-- removal is given a few seconds to come good rather than failing the test +-- that had already passed. A handle a test really leaks is still caught: +-- it is never released, and the wait runs out. +withTempDir :: String -> (FilePath -> IO a) -> IO a +withTempDir name body = do + tmp <- getTemporaryDirectory + let dir = tmp name + E.bracket (newDir dir) removeWhenItCan body + where + newDir dir = do + removeWhenItCan dir + createDirectory dir + return dir + removeWhenItCan dir = go (250 :: Int) + where + go 0 = removePathForcibly dir + go n = do + r <- E.try $ removePathForcibly dir + case r of + Right () -> return () + Left (_ :: E.IOException) -> threadDelay 20000 >> go (n - 1) diff --git a/test/HandshakeSpec.hs b/test/HandshakeSpec.hs --- a/test/HandshakeSpec.hs +++ b/test/HandshakeSpec.hs @@ -244,13 +244,22 @@ , ccUse0RTT = True } sent <- newEmptyMVar - withPipe (DropServerPacket [0 .. 50]) $ do - void $ forkIO $ void $ ignoreQUIC $ C.run cc $ \conn -> do + -- The client is held open past the send so that the connection does + -- not tear down before the take below, and killed with the test so + -- that it does not outlive it. Left to run out its own delay, it + -- went on sending Initial packets to the port the relay had just + -- given up, and the relay of whatever test came next latched onto + -- it: that test's client was then ignored for every datagram it + -- sent and failed on the idle timeout, ten seconds later and with + -- nothing to say why. + let client = ignoreQUIC $ C.run cc $ \conn -> do s <- stream conn sendStream s $ BS.replicate limit 97 putMVar sent () threadDelay 5000000 - Timeout.timeout 2000000 (takeMVar sent) `shouldReturn` Just () + withPipe (DropServerPacket [0 .. 50]) $ + E.bracket (forkIO client) killThread $ \_ -> + Timeout.timeout 2000000 (takeMVar sent) `shouldReturn` Just () where server = S.run sc $ \conn -> do s <- acceptStream conn diff --git a/test/IOSpec.hs b/test/IOSpec.hs --- a/test/IOSpec.hs +++ b/test/IOSpec.hs @@ -143,6 +143,8 @@ testFinalSize cc sc waitS [StreamF 0 0 ["ab"] True, StreamF 0 2 ["cd"] False] it "counts what the application never reads against the connection" $ do withPipe (DropClientPacket []) $ testCloseWithoutReading cc sc waitS + it "counts what arrives after the application has closed the stream" $ do + withPipe (DropClientPacket []) $ testCloseWhileArriving cc sc waitS it "counts a reset stream's final size against the connection" $ do withPipe (DropClientPacket []) $ testResetCountsAgainstTheWindow cc sc waitS describe "closing" $ do @@ -792,6 +794,40 @@ -- of the connection. Here twenty streams of four kilobytes go to a server -- that reads one octet of each, against a window of thirty-two: uncounted, -- the client is blocked before it is halfway through. +-- | The octets that arrive for a stream after the application has closed +-- it are given back to the connection's window too. +-- +-- They go nowhere -- the stream is gone from the table and the data is +-- dropped -- but the peer spent its connection window on them all the same. +-- Counted by nobody, the window we advertise falls that much behind what +-- the peer believes it has spent, for the rest of the connection. +-- +-- The stream is written in two halves with a pause between them, so that the +-- server reads its one octet and closes while the second half is still on +-- its way. Without the pause this is a race the server usually loses -- +-- which is why 'testCloseWithoutReading' passed on one machine and failed on +-- a slower one. +testCloseWhileArriving :: C.ClientConfig -> ServerConfig -> IO () -> IO () +testCloseWhileArriving cc sc0 waitS = + withAsync server $ \_ -> client + where + sc = sc0{scParameters = (scParameters sc0){initialMaxData = 32768}} + server = run sc $ \conn -> forever $ do + strm <- acceptStream conn + _ <- recvStream strm 1 + closeStream strm + client = do + waitS + r <- Timeout.timeout 5000000 $ C.run cc $ \conn -> + replicateM_ 20 $ do + strm <- stream conn + sendStream strm $ BS.replicate 2048 0 + threadDelay 2000 + sendStream strm $ BS.replicate 2048 0 + shutdownStream strm + closeStream strm + r `shouldBe` Just () + testCloseWithoutReading :: C.ClientConfig -> ServerConfig -> IO () -> IO () testCloseWithoutReading cc sc0 waitS = withAsync server $ \_ -> client diff --git a/test/LoggerSpec.hs b/test/LoggerSpec.hs --- a/test/LoggerSpec.hs +++ b/test/LoggerSpec.hs @@ -4,16 +4,21 @@ import qualified Control.Exception as E import Control.Monad (when) +import qualified Data.ByteString.Char8 as BS import Data.IORef +import qualified GHC.IO.Exception as E import GHC.IO.Handle (hDuplicate, hDuplicateTo) import System.Directory import System.FilePath import System.IO +import qualified System.IO.Error as E import System.Log.FastLogger (FastLogger) import Test.Hspec import Network.QUIC.Internal +import Config + spec :: Spec spec = do -- A debug logger is called from the protocol threads, six of which run @@ -27,19 +32,33 @@ -- and the peer saw a handshake that never finished. describe "stdoutLogger" $ it "drops a message stdout cannot take" $ - withUnwritableStdout (stdoutLogger "a line no one can receive") + onUnwritableStdout + (stdoutLogger "a line no one can receive") + (`shouldBe` ()) + + -- What the two above rest on, where a stdout that throws is not to be + -- had. + describe "dropIfUnwritable" $ do + it "drops an IOException" $ + dropIfUnwritable (E.throwIO $ userError "no space left on device") `shouldReturn` () + it "lets anything else through" $ + dropIfUnwritable (E.throwIO $ E.ErrorCall "boom") + `shouldThrow` errorCall "boom" describe "dirDebugLogger" $ -- The file is what the caller asked for, and it is still written -- when stdout is gone. it "writes the file when stdout cannot be written" $ - withDebugDir $ \dir -> do + withTempDir "quic-logger-spec" $ \dir -> do (dLog, clean) <- dirDebugLogger (Just dir) cid - withUnwritableStdout $ dLog "a line the file can take" - clean - readFile (dir show cid <> ".txt") - `shouldReturn` "a line the file can take\n" + onUnwritableStdout (dLog "a line the file can take") $ \() -> do + clean + -- Strictly: a lazy read leaves the handle open until the + -- content is demanded, and Windows will not delete a + -- file that is open. + BS.readFile (dir show cid <> ".txt") + `shouldReturn` "a line the file can take\n" -- The qlog writer is called from the sender, the receiver and the -- closer, the same protocol threads as the debug logger, so it must be -- no more able to end them. A qlog directory is asked for by name, as @@ -79,23 +98,42 @@ cid = makeCID "\x01\x02\x03\x04\x05\x06\x07\x08" -- | Running an action with a stdout every write throws on, and putting the --- real one back afterwards. stdout is redirected rather than closed: --- hspec reports through it, and a handle that is only redirected can be --- restored from the duplicate however the action ends. -withUnwritableStdout :: IO a -> IO a +-- real one back afterwards. 'Nothing' where stdout cannot be redirected +-- at all. +-- +-- stdout is redirected rather than closed: hspec reports through it, and a +-- handle that is only redirected can be restored from the duplicate however +-- the action ends. Any handle open for reading does for the stand-in, since +-- what makes the write throw is the mode GHC holds the handle in and not +-- anything the system does -- a file of our own rather than the null device, +-- which is \"\/dev\/null\" on one platform and \"NUL\" on another. +-- +-- 'hDuplicateTo' is @dup2@ on the device underneath, and the native handle +-- the Windows I\/O manager gives a standard stream does not implement it: +-- @dup2@ is left at its default, which throws. Asking the handle rather +-- than asking which operating system this is, because the handle is what the +-- test needs something of. +withUnwritableStdout :: IO a -> IO (Maybe a) withUnwritableStdout action = do + tmp <- getTemporaryDirectory + let file = tmp "quic-logger-spec-readable" + writeFile file "" saved <- hDuplicate stdout - let redirected = withFile "/dev/null" ReadMode $ \h -> do - hDuplicateTo h stdout - action `E.finally` hDuplicateTo saved stdout + let redirected = withFile file ReadMode $ \h -> do + swapped <- E.try $ hDuplicateTo h stdout + case swapped of + Left e + | E.ioeGetErrorType e == E.UnsupportedOperation -> + return Nothing + | otherwise -> E.throwIO e + Right () -> + Just <$> action `E.finally` hDuplicateTo saved stdout redirected `E.finally` hClose saved -withDebugDir :: (FilePath -> IO a) -> IO a -withDebugDir = E.bracket newDir removePathForcibly - where - newDir = do - tmp <- getTemporaryDirectory - let dir = tmp "quic-logger-spec" - removePathForcibly dir - createDirectory dir - return dir +-- | Running what needs a stdout that throws, or saying why it was not run. +onUnwritableStdout :: IO a -> (a -> Expectation) -> Expectation +onUnwritableStdout action check = do + r <- withUnwritableStdout action + case r of + Nothing -> pendingWith "stdout cannot be redirected on this handle" + Just x -> check x diff --git a/test/QLoggerSpec.hs b/test/QLoggerSpec.hs --- a/test/QLoggerSpec.hs +++ b/test/QLoggerSpec.hs @@ -3,12 +3,13 @@ module QLoggerSpec where import qualified Control.Exception as E -import System.Directory -import System.FilePath +import System.Directory (listDirectory) import Test.Hspec import Network.QUIC.Internal +import Config + spec :: Spec spec = do describe "dirQLogger" $ do @@ -19,7 +20,7 @@ -- exclusively: the second to ask got "openFile: resource busy" thrown -- through its connection setup rather than a worse log. it "gives the two ends of one connection their own files" $ - withTempDir $ \dir -> do + withTempDir "quic-qlogger-spec" $ \dir -> do now <- getTimeMicrosecond let cid = toCID "01234567" E.bracket (dirQLogger (Just dir) now cid "client") snd $ \_ -> @@ -27,10 +28,3 @@ return () files <- listDirectory dir length files `shouldBe` 2 - -withTempDir :: (FilePath -> IO a) -> IO a -withTempDir body = do - tmp <- getTemporaryDirectory - let dir = tmp "quic-qlogger-spec" - E.bracket_ (createDirectoryIfMissing True dir) (removeDirectoryRecursive dir) $ - body dir diff --git a/test/SetupSpec.hs b/test/SetupSpec.hs --- a/test/SetupSpec.hs +++ b/test/SetupSpec.hs @@ -18,7 +18,7 @@ import Control.Concurrent import Control.Concurrent.Async import qualified Control.Exception as E -import System.Directory +import System.Directory (createDirectory, listDirectory) import System.FilePath import System.IO import Test.Hspec @@ -38,7 +38,7 @@ -- pointed at one directory. describe "a server connection setup that fails" $ it "does not leave the qlog it had already opened open" $ - withTempDir $ \dir -> do + withTempDir "quic-setup-spec" $ \dir -> do let qdir = dir "qlog" createDirectory qdir sc0 <- makeTestServerConfig @@ -91,13 +91,3 @@ case r of Right () -> return () Left (_ :: E.IOException) -> threadDelay 20000 >> go (n - 1) - -withTempDir :: (FilePath -> IO a) -> IO a -withTempDir = E.bracket newDir removePathForcibly - where - newDir = do - tmp <- getTemporaryDirectory - let dir = tmp "quic-setup-spec" - removePathForcibly dir - createDirectory dir - return dir