Content-Encoding gzip doesn't get decoded properly about fly HOT 22 CLOSED

Can I work on this one?

from fly.

Sure you can. It's a bit more involved than the other ones you've been working on.

I'm not sure this is a "bug" (as its been labeled.) I think people might want to actually get the gzip compressed (deflated) data without decompressing (inflating) it. gzip inflation can also incur a somewhat heavy cost.

I think what we want to do is make that a feature. Allow for users to optionally specify if they want gzip content to be inflated. Or maybe that's the default, I'm not sure.

I would probably design it as an off-spec, extra option to fetch, like:

fetch("http://example.com", { headers: {"Accept-Encoding": "gzip"}, inflate: true })

Then on our side, we would forward that option to the ProxyStream we send back. The latter would know that it should transform its stream using zlib to inflate the body as it's being read.

^ We only want to do that if the Content-Encoding header contains the string gzip though.

This will require end-to-end testing.

from fly.

Sounds good. I will start working on that. Should we use something like

Buffer.from(gzipData, 'base64').toString()

Or a different library? It seems like zlib would be the best option

Todos:

• Add fetch options
• Implement decoding
• Document options
• Add end-to-end testing

from fly.

You'll need zlib from node.js to inflate. You'll need to look into how streams work in general. There are examples a bit everywhere on how to use node.js to inflate a gzipped stream of data using zlib :)

gzip isn't base64-encoded, it's just compressed raw data. Also, we want to avoid buffering data, so you'll definitely need streams.

Furthermore, the result could be any kind of data, not just a string. It won't matter though, this should work seamlessly with the way we use bodies in fly. As long as the ProxyStream is "transforming" the original stream into an inflated stream via zlib, we're good.

from fly.

Awesome. I will start on that. I think I will need to modify the fetch function, where would I find that?

from fly.

It's here: https://github.com/superfly/fly/blob/master/v8env/fetch.js#L16

Everything running inside non-default v8 is in the folder v8env/.

from fly.

Ahh, should have thought to look there. Thanks.

from fly.

I have been trying to figure this out a little more, and I think I could get it with a little help. That being said, I also understand if you guys have other things to be doing and don't have time to explain this all to me.

If you do have time to help me, I think I understand at least the basics of what will need to happen. First, I added a variable to fetch that checks if inflate is set to true. When fetch makes a request, if it is an ivm reference _applyFetch will create a refToStream out of the nodeBody (in this case the nodeBody would be gzipped data). Then in refToStream what happens is a stream is created to get all the data from the fetch response. This is where I would need to implement the inflation of the data.

For implementing the inflation of the data, I would need to do something with the ReadableStream that is returned in refToStream. I think I can probably figure that out from the docs. Something like this is my guess:

zlib.unzip(data, (err, buffer) => //do something with `buffer`

After the data has been unzipped I think I would add it to the controller (I think controller.enqueue(data) would do this).

Thats about as far as I have gotten (if that even makes any sense) and I have no idea if that is how it should be done, but I thought I would check before I start on anything.

Thanks.

from fly.

We're totally happy to help.

I think you're really close. Have you wrapped your head about the difference between v8env and the node/bridge native stuff yet?

You should be able to do this entirely in the fetch bridge function: https://github.com/superfly/fly/blob/master/src/bridge/fetch.ts#L148

On that line, we take the res, which is stream-like, and wrap it in ProxyStream to send back to the v8env. Since zlib.unzip already is meant to work on streams, it might be as simple as just wrapping ref before you create a ProxyStream.

Something like:

let body = res
if(inflate){
  body = zlib.unzip(body)
}

Then pass body to the ProxyStream constructor instead of res.

from fly.

Thanks! That is definitely easier than I expected.

As for the difference between v8env and node/bridge native stuff, I can't say I fully understand what the difference between the two are.

from fly.

@mrkurt I was looking at the wrong file. I thought it was somewhere in https://github.com/superfly/fly/blob/master/v8env/fetch.js

from fly.

Alright! The tldr is that v8env is where user application code runs, it's the custom v8 environment we provide for apps. The bridge/native code is where we let that interact with the outside world. When a user's app calls fetch, they hit a v8env function. That calls across the "bridge" to the native function that actually does the i/o.

Now that I look at it, I realized you'll probably need to tweak the v8env fetch code to allow the inflate option as well.

So what you were looking at was basically the shim function that offloads all the work to the bridge.

from fly.

Okay - that actually makes a lot of sense now.

from fly.

@mrkurt res is an instance of incoming message. I am not sure if there is a way to set the 'body' of an incoming message and ProxyStream can't take just a string or buffer.

from fly.

I think I figured something out. Its a bit ugly, but should work:

      if(inflate && typeof body == 'string'){
        zlib.unzip(body, (error, buffer) => {
          if (error) throw Error ('')

          let stream = new Readable()
          stream.push(buffer)
          stream.push(null)
          ctx.applyCallback(cb, [
            null,
            new ivm.ExternalCopy({
              status: res.statusCode,
              statusText: res.statusMessage,
              ok: res.statusCode && res.statusCode >= 200 && res.statusCode < 400,
              url: urlStr,
              headers: res.headers
            }).copyInto({ release: true }),
            res.method === 'GET' || res.method === 'HEAD' ? null : new ProxyStream(stream).ref
          ])

          return
        })
      }

from fly.

after #54 goes through I think I will submit a PR and if other stuff needs to be worked out we can do it there, does that work?

from fly.

Yep! We can work off a PR.

ProxyStream accepts any Readable, it doesn't have to be just a response. The TypeScript might be throwing you, try this:

let body: Readable = res
body = zlib.unzip(res)

The way we're using res, we pass it separately. That previous ivm.ExternalCopy sends all the headers over, the ProxyStream is just the body portion.

from fly.

Yeah, digging into it a little more I saw that. Thats why I created a new Readable() and pushed the buffer to it - I don't think zlib.unzip returns a Readable.

Anyway, I will submit the PR after the other one gets merged (I don't think its a good idea to have two PRs at the same time).

from fly.

I'm ok with two PRs! It would be best if you make a clean branch off of master to make a new PR with, though.

from fly.

Okay, sounds good - will do.

from fly.

After playing around with it a little more, I am getting this error when actually trying to decode something:

Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

from fly.

We've settled on having people not send the Accept-Encoding: gzip header and hoping the server is properly behaved. We might add an option to inflate later on though.

from fly.