braid-org / braidjs

Monorepo for Braid Projects in Javascript
https://braid.org
262 stars 15 forks source link

braid_fetch issue on Firefox #15

Closed JonhyOliveira closed 2 years ago

JonhyOliveira commented 2 years ago

Hey everyone, I think I found a bug in the braidify library regarding how subscription data is parsed on the client side. On the client-side: image

On the server-side: image

Here is a snippet from braidify-client where the subscription starts getting parsed. res.body gets passed as a stream to the stream handler Screenshot_20220316_174640

Inside the stream handler a new subscription_parser object is instantiated with the property state set to its default value { input: "" }. The problem is that inside the parser the state.headers property is never defined so parse_version will try to parse headers from the input (res.body, which obviously doesn't contain header information).

image image

Thus, if you look through the code there is no other place where the callback (passed to handle_fetch_stream, see first image) is called with result != null, besides when parser's state.result === success, which never happens because we can't parse header information from the body.

A quick fix is passing a subscription_parser object directly into handle_fetch_stream with state.headers defined to the res.header property. This is just the first idea that came into my mind, feel free to leave below a better solution. I'll show my implementation here when I figure out how to push it to this issue :)

Edit1: was incomplete Edit2: typo

JonhyOliveira commented 2 years ago

And voilá, the content arrived :man_dancing: image

toomim commented 2 years ago

Hey @JonhyOliveira, thanks for this bug report, and the fix!

Can you share the code that reproduces the problem? I've been unable to reproduce it. I wonder if there's something in my config that's different than yours, and would like to get to the bottom of it.

toomim commented 2 years ago

The problem is that inside the parser the state.headers property is never defined so parse_version will try to parse headers from the input (res.body, which obviously doesn't contain header information).

Braid-HTTP actually does send headers over res.body, within a subscription. This is how a subscription works.

A subscription sends multiple sub-responses (each with its own headers and body) within a single response body. (See https://datatracker.ietf.org/doc/html/draft-toomim-httpbis-braid-http#section-3.2) For example:

   Response:

         HTTP/1.1 209 Subscription
         Subscribe: keep-alive                                 | Response Headers

         Version: "ej4lhb9z78"                                 | Version headers
         Parents: "oakwn5b8qh", "uc9zwhw7mf"                   |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Content-Length: 73                                    |

         [{text: "Hi, everyone!",                              | Version body
           author: {type: "link", value: "/user/tommy"}}]      |

         Version: "g09ur8z74r"                                 | Version headers
         Parents: "ej4lhb9z78"                                 |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Patches: 1                                            |

         Content-Length: 62                                    | Patch headers
         Content-Range: json .messages[1:1]                    |

         [{text: "Yo!",                                        | Patch body
           author: {type: "link", value: "/user/yobot"}]       |

All of the Version and Patch headers and bodies are encoded within the normal HTTP body, so they are all found within res.body in the code you're looking at.

toomim commented 2 years ago

I just added some test code here: https://github.com/braid-org/braidjs/tree/master/braidify/test

See if that works for you.

JonhyOliveira commented 2 years ago

Hey @toomim, my initial problem was that I was getting a NetworkError, while debugging it seems I have diverged a lot from the base code and actually introduced some bugs! Anyway, I ran your stuff with the upstream version of braidify_client and.. got another NetworkError, but I tried running it on Chromium (my main browser is Firefox) and it ran just fine. I've added the free_the_cors method from braidiy/demos/blog3/server.js, but that didn't do it either, so now I'm confused.

image image

Also it's important to note that the request IS arriving at the server, and its sending back the version, which strengthens the idea that the issue I'm having is with Firefox.

Anyway, it's a bit late here, so I'll have a quick look over the Mozilla documentation of their CORS stuff. If anything comes to mind let me know and I'll update here once I find a solution.

JonhyOliveira commented 2 years ago

Alrighty, I don't have any solution yet, but I've ran some tests and found that if I keep flooding the parser it eventually spits out everything (like there is some kind of buffer?) Very odd behavior for me, because even-tough I get a NetworkError it seems like the connection actually was established.

Note: It seems interesting to me that it's consistently spitting out everything at the 8th version

JonhyOliveira commented 2 years ago

I have my code here, if you want to have a look. Currently on Firefox 88.0.1

toomim commented 2 years ago

Oh, that's awesome info! Thanks @JonhyOliveira! I've heard of some proxies buffering long responses; but I didn't realize firefox could do this! I also recall hearing some solutions people use (like flooding extra data over the connection to flush a buffer) and have been wanting a chance to try them out! I'll try Firefox and see if I can reproduce this.

toomim commented 2 years ago

I have a working workaround for Firefox: add some extra blank lines after each new version. If there are enough, it seems to flush the cache for firefox.

Try making this one-line change in braidify-server.js:243, change:

    if (res.isSubscription)
        res.write("\r\n")

into:

    if (res.isSubscription)
        for (var i=0; i<256; i++)
            res.write("\r\n")

The firefox client is still declaring that there was some "Network error", but it's successfully receiving the version!

image

The minimum number of blank lines I need for the buffer to flush is 221.5, which corresponds to a body length of 583 bytes. More concretely, this is the smallest successful string:

"version: \"test\"\r\nparents: \r\ncontent-length: 20\r\n\r\n{\"list\":{\"list\":[]}}\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r"

toomim commented 2 years ago

This technique of shoving extra \r\n newlines down the pipe is based on the discussion from https://github.com/braid-org/braid-spec/issues/73. @josephg might be interested.

toomim commented 2 years ago

Here's the source of the Firefox "Error in body stream":

  // Let's use a generic error.
  ErrorResult rv;
  // XXXbz can we come up with a better error message here to tell the
  // consumer what went wrong?
  rv.ThrowTypeError("Error in body stream");

...on line 524 of BodyStream.cpp in Firefox.

toomim commented 2 years ago

Ok, I've updated braidify to detect Firefox and emit extra \r\n to work around this bug: https://github.com/braid-org/braidjs/commit/b148b9caf2e0dbb759efc2eb4d32f6d83efd66e8

toomim commented 2 years ago

This workaround has been published to npm's braidify.

JonhyOliveira commented 2 years ago

Awesome, there seems to be some buffoonery going on Firefox's side. I liked your idea of just flushing empty lines, but this will hurt performance of the Protocol. I still wonder what might be causing this.. normal fetches seemed to work just fine, maybe it's the open connection?

JonhyOliveira commented 2 years ago

I closed the connection with res.end() from the server-side right after sending the version and surprise, surprise: No error!

Screenshot_20220317_095001

Looks like Firefox doesn't like passive fetches, maybe it would be a good idea to open an Issue on their repo?

JonhyOliveira commented 2 years ago

Just found out they don't have a repo, but I will submit a bug report on this :)

Also, I'll submit this as a feature request, since as I understand it Firefox doesn't like it because the normal fetch is supposed to return a Response object.

Edit: Link to the bug report, where we can continue this discussion. Also, I actually submitted it as a bug report :P. Looking back on it, it may have been the better choice, since this behavior is inconsistent with what you have experienced so far on your testing (not using Firefox I assume).

josephg commented 2 years ago

I'm confused by this issue because braid-protocol doesn't do this, and doesn't have any problems in firefox. Eg, My (DT wiki prototype)[https://wiki.seph.codes/] works over braid-protocol and works fine in FF without any of this.

@JonhyOliveira is there anything else needed to replicate this problem?

toomim commented 2 years ago

Braid-protocol might display this bug, too. You don't know until you test it.

A reproducible test-case is linked above: https://github.com/braid-org/braidjs/tree/master/braidify/test

The hypothesis is that firefox fails when there isn't enough data going over the wire. I'd suggest sending the equivalent data with braid-protocol and seeing if it works.

toomim commented 2 years ago

Here, I've captured the request and response test case from/to firefox:

Request:

GET /json HTTP/1.1
Host: localhost:9000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Firefox/91.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: http://localhost:9000/
peer: g8nc6gi3fd9
subscribe: true
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin

Response:

HTTP/1.1 209 unknown
Range-Request-Allow-Methods: PATCH, PUT
Range-Request-Allow-Units: json
Patches: OK
subscribe: true
cache-control: no-cache, no-transform
Date: Fri, 18 Mar 2022 02:49:46 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Transfer-Encoding: chunked

11
version: "test"

b
parents: 

14
content-length: 16

14

{"this":"stuff"}

2
josephg commented 2 years ago

Thanks I’ll give that a try!

toomim commented 2 years ago

You can capture a request in unix with nc -l 9000 > request.txt to listen to port 9000 while your browser initiates a request, and then capture a response with nc localhost 9000 < request.txt to read the request from disk and send it to a server running on port 9000.

toomim commented 2 years ago

I'm trying to debug the TypeError: Error in body stream exception now. This doesn't break anything, since it only fires after a connection dies; but it'd be nice to not see the red line in the console.

It appears in firefox using the testcase above, but not in firefox on wiki.seph.codes. Strangely, it appears in the JS console after you reload the page—not while the page is running.

To isolate the cause of the exception, I swapped the clients and servers of braid-protocol and braidify. The error occurred when using braidify-client.js, on either server, and did not occur with the braid-protocol client, so I've narrowed it to something in braidify-client.js.

It appears to be thrown from the call to response.body.getReader().read() on line 327:

var {done, value} = await reader.read()

This sits and waits for the next chunk of data to come from the server, and when the connection ends (only on firefox, only when using braidify-client.js library), it throws the exception "TypeError: Error in body stream".

toomim commented 2 years ago

Hm, I've now eliminated braidify-client, and replaced it with this minimal fetch() test-case, and I'm still seeing the "Error in body stream" exception in Firefox when the subscription ends:

<script>
  fetch('/json', {headers: {subscribe: true}})
    .then( res => res.body )
    .then( async body => {
        var reader = body.getReader()
        while (true) {
            var result = await reader.read()
            console.log(`Got ${result.value.length} bytes`)
        }
    })
</script>

That's just a normal fetch(). Apparently firefox doesn't like it when a normal fetch connection ends.

The next step to solving this would be to make a minimal fetch() in the braid-protocol style that doesn't trigger this exception, to see what the difference is.

JonhyOliveira commented 2 years ago

Nice, thank you for investigating this further, but you are not catching any errors. Could that eliminate the "Error in body stream"? On Sat, Apr 9, 2022, 00:01 Michael Toomim @.***> wrote:

Hm, I've now eliminated braidify-client, and replaced it with this minimal fetch() test-case, and I'm still seeing the "Error in body stream" exception in Firefox when the subscription ends:

That's just a normal fetch(). Apparently firefox doesn't like it when a normal fetch connection ends.

— Reply to this email directly, view it on GitHub https://github.com/braid-org/braidjs/issues/15#issuecomment-1093449391, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM6YIATL7IYJDBSA57OY3V3VEC3DLANCNFSM5Q4WRN4A . You are receiving this because you were mentioned.Message ID: @.***>

josephg commented 2 years ago

Yeah I was going to make the same point. It sounds like fetch throws an error when the connection dies. That makes sense. Your braidify code should catch it and then try and then enter a reconnection loop. Either fetch().then(…).catch(() => …) or try { await fetch() } catch e { … }

toomim commented 2 years ago

Yeah, after I posted this I realized that I was chasing a non-bug.

Braidify does actually catch the error and pass it through to the application, and making the application catch it in this case makes the bug go away:

Revised test/client.html:

  // Do a braidified fetch
  fetch(url, {subscribe: true})
     // Print each update to the console
     .andThen((version) => {
         document.writeln('<pre>We got ' + JSON.stringify(version) + '!')

     // Ignore network disconnects
     }).catch( e => {} )

The reason this was confusing was that firefox shows you the previous page's exceptions when you reload the page. All other browsers just throw away exceptions that occur from a previous page load.

And I suspect that the reason this doesn't show up in the braid-protocol client is that it just catches the error in all the apps I tested it with.

toomim commented 2 years ago

In other words, if you replace the e => {} with e => {console.log('bloop!')} then you'll see the bloop! appear after you reload the page, on a page load that doesn't ever throw that error.

Firefox runs the handlers wayyy late.

toomim commented 2 years ago

Thank you both for following along and helping out.