improve fake network write() efficiency #1178
Conversation
- new: class EthernetPacketEncoder, replaces all dynamically allocated write-buffers with a single, static buffer - new: class Uint8Stream, designed to replace TCPConnection.send_buffer[] with a dynamically growing ringbuffer - new: added internet checksum to synthesized UDP packets (required by mTCP) in function write_udp() - replaced internet checksum calculation with more efficient algorithm from RFC1071 (chapter 4.1) - increased TCP chunk size in TCPConnection.pump() from 500 to 1460 bytes (max. TCP payload size) - added function to efficiently copy an array into a DataView using TypedArray.set(), replaces a bunch of for-loops - added function to efficiently encode a string into a DataView using TextEncoder.encodeInto() - refactored all write_...()-functions to use EthernetPacketEncoder - added several named constants for ethernet/ipv4/udp/tcp-related offsets - removed several apparently unused "return true" statements
TCP FIN flag marks the sender's last package. This patch delays sending FIN in TCPConnection.close() until the send buffer is drained.
- Renamed Uint8Stream to GrowableRingbuffer - Minimum capacity now guaranteed to be 16, and maximum (if given) to be greater or equal to minumum - Changed peek(dst_array, length) to peek(dst_array) - Removed misleading method resize(), moved code into write() - Now throws an Exception on capacity overflow error instead of console.error() - Removed debug code that is no longer needed When we have a capacity overflow the situation is as bad as if the Uint8Array constructor had failed with out-of-memory, which throws a RangeError.
- use actual dynamic TCP port range from RFC6335 - throw an Exception when pool of dynamic ports is exhausted
- removed class EthernetPacketEncoder and its global single instance ethernet_encoder
- moved encoder buffer ownership to adapter (WispNetworkAdapter and FetchNetworkAdapter)
- made view_{setArray,setString,setInetChecksum} global functions
- changed prototype of all write_...(spec, ...) functions to uniform write_...(spec, out)
- removed function make_packet(spec), added new function adapter_receive(adapter, spec)
- replaced all calls of form "A.receive(make_packet(S))" with "adapter_receive(A, S)" (also in WispNetworkAdapter)
Note that before this patch function make_packet() was always called in the form:
adapter.receive(make_packet(spec));
This patch binds the lifetime and scope of the encoder buffer from class EthernetPacketEncoder to the WispNetworkAdapter/FetchNetworkAdapter instance "adapter".
- removed unnecessary comments around TCP_STATE_... - renamed TCPConnection.send_stream to send_buffer - calculate package length in write_udp() and write_tcp() along the same structure as in all other write_...() functions
For the most part, this patch adds graceful TCP connection shutdown behaviour to class TCPConnection in fake_network.js. Network-intense guest applications like "apt" run noticeably smoother with support for graceful TCP shutdown. Changes in detail: - completed SYN-related state mechanics (TCP connection establishment) - added FIN-related state mechanics (TCP connection shutdown) - reordered and regrouped TCP packet handling steps in TCPConnection.process() - reduced minimum IPv4 packet size from 46 to 40 bytes to reduce noise in logs (why is this even checked) - added explicit detection of TCP Keep-Alive packets to reduce noise in logs - added dbg_log() calls to trace TCP connection state (could be removed if unwanted, or maybe use smth. like LOG_TCP instead of LOG_FETCH?) - removed TCPConnection.seq_history as it was not used anywhere As should be expected, the changes related to TCP connection state only affect class TCPConnection internally and are mostly concentrated in its methods process() and pump(). A few TCP connection states are left unimplemented, reasons: - CLOSING: simultaneous close from both ends (deemed too unlikely) - TIME_WAIT: wait seconds to minutes after sending final ACK before entering CLOSED state (pointless in our specific case and would require a Timer) - LISTEN: are server sockets even supported? Both WISP and FETCH do not support them. Also, TCPConnection.connect() seems not to be called from anywhere (it used to be called from fake_tcp_connect() which was deleted in the previous patcH). As before, fake_network.js does not use any Timers (i.e. setTimeout()), the code is purely driven by outside callers. That means that there are no checks for lost packets (or resends), the internal network is assumed to be ideal (as was the case before). Even though I fixed and removed most of the TODOs I originally found in fake_network.js, I've added several new ones that are left to be discussed. Tested with V86 network providers WispNetworkAdapter and FetchNetworkAdapter, and with V86 network devices ne2k (FreeDOS) and virtio (Debian 12 32-bit).
- removed orphaned function siptolong() - refactored view_setInetChecksum() to calc_inet_checksum() - renamed view_setString() into view_set_string() and simplified it - renamed view_setArray() into view_set_array() - refactored adapter_receive() to make_packet() - bugfix: send FIN after receiving last ACK instead of tagging it to the last packet we send, fixes a corner case when TCPConnection.close() is called with empty send_buffer but with a yet to be acknowledged package in transit. Send a FIN packet in TCPConnection.close() only if both the send_buffer is empty and there is no package in transit, else delay that until we receive the last ACK.
|
I have a small idea for fetch-based networking that I would first like to check with you before committing. Currently, the full HTTP response (header and body) from fetch() is cached before being forwarded to the originating HTTP client. My proposal is to only wait for the HTTP response headers from fetch(), and then to forward the response body asynchronously chunk by chunk to the originating HTTP client using ReadableStream and HTTP Chunked transfer encoding. To test this, I changed on_data_http() in fetch_network.js based on a ReadableStream usage pattern from MDN, it's pretty straight-forward and it works well with my tests (all HTTP applications support chunked Transfer-Encoding to my knowledge). This would also remove the need for FetchNetworkAdapter.fetch(), I merged its relevant code into on_data_http(). The upside is that this much, much increases both the responsiveness as well as the effective speed of fetch-based networking, all the more with increasing size of the HTTP response. The downside is that the originating HTTP client never gets to see the Content-Length of the response it is receiving, thus it cannot show any meaningful progress statistics anymore other than about what it has received so far. Any Content-Length must be ignored if any Transfer-Encoding is used, and the Content-Length from the fetch response header can be incorrect (for example, for compressed response bodies). The only way around this seems to be the prefetching like it currently is. I might be wrong, but to my knowledge it's categorically either prefetching or the loss of Content-Length (for any request from the originating HTTP client). I wasn't aware of that until I had finished implementing it. I would of course wait with this until #1182 is merged, and include any changes from there. I'm really undecided which side outweighs the other here. What do you think, thumbs up or down? |
Generally thumbs up, but I'm unsure about the downsides. I wonder if HTTP Chunked transfer encoding is necessary at all. Can't the data be streamed in separate ethernet packets?
I believe for compressed bodies (via Content-Encoding), |
|
EDIT: The gist of the matter is that I cannot determine whether If I could rely on I build and send the response headers to the originating client when I receive the first response body chunk, but I don't have a In fetch(), my browser negotiates gzip compression with my Apache server and transparently decompresses the response before passing me the decompressed chunks (and passes me the compressed size in Content-Length, which is useless). I never get to know how long the response is going to be myself. I tried to suppress compression but I wonder, is there any other method besides Chunked transfer encoding that I could use when I don't know the length of the response? |
I see the problem now.
It seems to be legal to send an http reponse with neither |
I actually do all this to learn and also try out new things, so thank you very much for this information! It should fit in well since I'm already closing the HTTP connection to the originating client once the transaction is complete (current fetch-based networking can't do that because of the FIN packet that is sent too early). |
|
It works without both Content-Length and chunked encoding! :) Of course, the originating client still doesn't get to learn the Content-Length of the response body, so it cannot show any meaningfull download progress information besides the total bytes received so far. |
See https://en.wikipedia.org/wiki/File:Tcp_state_diagram_fixed_new.svg for a full TCP state diagram, all states (except TIME_WAIT) and state transitions below ESTABLISHED are now properly implemented in class TCPConnection. - fixed misconceptions regarding TCP close states and transitions - fixed incorrect behaviour when guest closes a TCP connection - added state CLOSING to state machine - added support to pass passive close up to WispNetworkAdapter - commented out all calls to dbg_log() that are part of this PR This improves interoperability by further completing the TCP state machine model in fake_network.js.
|
Looks good to merge to me (one more small comment above). @chschnell Any objections? (asking because it's still marked as a draft) |
Only WispServerCpp implements CLOSE/0x02 like that, neither epoxy-tls nor wisp-js do, in my observation. There was no CLOSE/0x02 in V86's WISP client before I added it a couple of days ago. So in the case of WispServerCpp that commit did remove full closing, but in the case of the other two WISP servers it didn't change anything. I did that because none of them implemented half-close. Now it all comes down to a timeout by the server, or a leaked connection in case that never happens. Maybe I should put the CLOSE/0x02 better back in, even though it will break certain applications and has no effect with some WISP servers. |
|
Are you saying the other wisp servers don't support closing or don't support half closing? Sorry I think I'm misunderstanding To my understanding every current wisp server does support full closing of streams from either direction (client and server can both report closed streams I believe) Also close is 0x04, the reason is 0x02. Just wanted to clarify because I stated it incorrectly above in my own message |
The protocol does not specify any special behavior on the server side for specific close reasons. I don't know why WispServerCpp behaves differently here, but wisp-js and epoxy-server are following protocol and closing fully.
On epoxy, you can use |
|
Also wisp server Cpp sends a 0x04 Close packet back after the client sends one. It is the only server to do this, and it actually isn't in the spec. The client is supposed to assume the stream is closed immediately after sending the close packet. Just wanted to note that on this GitHub thread |
|
Another note: WispServerCpp doesn't actually close any sockets that it forwards, violating spec, so the remote server never sees the close from the wisp client. |
- both methods must be overloaded by the network adapter, just like TCPConnection.on_data() - on_shutdown() informs the network adapter that the client has closed its half of the connection (FIN) - on_close() informs the network adapter that the client has fully closed the connection (RST) - added call to on_shutdown() when we receive a TCP packet with active FIN flag from guest - added call to on_close() when we receive a TCP packet with active RST flag from guest - added calls to on_close() when we have to tear down the connection in fake_network.js - added implementation of on_shutdown() and on_close() in wisp_network.js The default implementation of these methods is to do nothing. These methods do not apply to fetch-based networking, fetch() only supports HTTP and it doesn't matter if the client closes its end of the connection after it has sent its HTTP request. Hence FetchNetworkAdapter does not override these methods. Note that WISP currently only supports close(), as a workaround shutdown() is implemented like close() which might be incorrect (missing support for half-closed TCP connections).
|
I think it's worth looking into making a protocol extension for half closing, @ading2210 @r58Playz |
Sorry about the confusion. I meant to say the other WISP servers don't support closing, but it turned out I was wrong about that.
Understood. By 0x02 I always meant reason 0x02 (Voluntary stream closure).
Thanks, I enabled tracing and indeed, I see the same message. Sorry about my confusion, I was wrong when I said it wasn't supported by epoxy-tls/wisp-js, they indeed do. |
|
My latest commit is a slight change in strategy with this close issue. I now distingush between on_shutdown() (FIN) and on_close() (RST), this is the right way to model it in fake_network.js. I implemented both on_shutdown() and on_close() for WISP using a CLOSE frame with reason 0x02. This does not implement half-closed TCP connections (currently not supported), but this also doesn't leak idle TCP connections. It would be trivial to change on_shutdown() in case WISP support changes in the future. |
|
Thanks! This does make retrofitting it migh easier if the server advertises support for TCP half closing. |
…apter.send() - in both functions, all the scattered "if" statements are mutually exclusive - changed control flow to mutual exclusive cases - removed boolean return values in handle_fake_networking(), they were never checked by the caller
- added missing callback WispNetworkAdapter.on_tcp_connection(packet, tuple), moved all relevant code from WispNetworkAdapter.send(data) to new method, replaced with call to handle_fake_tcp() (now identical to FetchNetworkAdapter) - removed redundant first argument of adapter.on_tcp_connection(adapter, ...) - added TCP_STATE_SYN_RECEIVED -> TCP_STATE_ESTABLISHED state transition back in (just to be sure) The larger goal is to replace all code in WispNetworkAdapter.send(data) with a call to handle_fake_networking(), just like in FetchNetworkAdapter. The only difference left is two alternate fake DNS implementations.
|
Currently there are two different fake DNS methods in V86:
Fetch-based networking uses the first method, wisp-based the second, and I'm not sure why. Faking DNS with DoH works just fine for both fetch- and wisp-based networking, and it returns real results. So I would like to move the DoH-code from wisp_network.js into handle_fake_dns() (fake_network.js) and make it the default fake DNS method. Question: Is the first method without DoH still needed or can it be removed? I think it is not needed, only fetch-based networking uses it, and with fetch() this DNS method doesn't really matter (the browser performs the DNS lookup for the URL passed to fetch()). In case I should keep the first method I would suggest to add a new option |
For fetch-based networking, there is not much need for it: #1061 (comment), and fetch with IP address works buggy, especially if HTTPS-only mode is enabled in browser. |
|
Fetch networking uses the Host: header so the IP address given is irrelevant, to save time it just returns a dummy IP. Since wisp actually connects to the IP it has to do the real resolution |
|
Sorry just saw your command supermaxusa, my Internet is glitching |
|
I don't think it makes much sense to add Doh to fetch personally since it's not really necessary based on how it operates (unless you're running a SUPER OLD pre Host header http client). But I do think the Doh server should be configurable |
- added member "dns_method" to both FetchNetworkAdapter (value "static") and WispNetworkAdapter (value "doh") - moved the DoH-code from WispNetworkAdapter.send() into new function handle_fake_dns_doh() in fake_network.js - renamed function handle_fake_dns() into handle_fake_dns_static() - recreated function handle_fake_dns() that now calls either of the two depending on the value of adapter.dns_method
|
@SuperMaxusa @ProgrammerIn-wonderland Thanks a lot for your input! We all agree that DoH doesn't make much sense with fetch-based networking, my point was that it also doesn't hurt because it matters so little. But it's fine, I'll leave it at that. In my latest commit I moved the DoH-code from WispNetworkAdapter.send() into new function The benefit of all this is that all of the code in WispNetworkAdapter.send() that was duplicated from fake_network.js is now gone which somewhat simplifies reasoning about the fake networking control flow (it made me scratch my head quite a little). |
|
Thanks! This looks good to merge to me now. Let me know if there's another change you'd like to get in. I will try to rebase (to get rid of the merge commits). |
Yes, let's merge it. I see two things left to do, but I want to split them off into a new PR in a couple of weeks.
I will do other things in the next week or two, but I think then I'll take another go at this, if you don't mind. |
|
Great work! A huge improvement over my hacked up proof of concept. |
My pleasure! And it was just a bit rough around the edges, the core was rock-solid. |
|
Merged manually, thanks all! |
|
@copy I am very sorry but I just noticed that I broke the |
|
@chschnell Sure |
Several improvements in fake network code that deal with data flowing towards the guest OS (i.e. package encoding and downstream buffering) and TCP state mechanics.