Skip to content

Conversation

sgasho
Copy link

@sgasho sgasho commented Jun 14, 2025

related: denoland/deno#29281

enabled gzip decoder to accept an empty gzip.
I added some test cases for this fix at tests/gzip.rs

reproduction

  1. clone deno
  2. cargo build --config 'patch.crates-io.async-compression.path="../async-compression"'
  3. prepare server_hasdata.js & server_nodata.js
  4. prepare client.js
  5. /path/to/deno-debug --allow-net client.js

server_hasdata.js

import { serve } from "https://deno.land/[email protected]/http/server.ts";

serve(() => {
  const encoder = new TextEncoder();
  const payload = encoder.encode("Hello Deno!");

  const gzipStream = new ReadableStream({
    start(controller) {
      controller.enqueue(payload);
      controller.close();
    },
  }).pipeThrough(new CompressionStream("gzip"));

  return new Response(gzipStream, {
    headers: {
      "Content-Encoding": "gzip",
    },
  });
});

server_nodata.js

Deno.serve((req) => {
    return new Response(new Uint8Array(), {
        headers: {
            'Content-Encoding': 'gzip',
        },
    });
});

client.js

const result = await fetch('http://localhost:8000/').then(r => r.text());
console.log(result);

result

  • no errors with server_nodata.js(used to have an "unexpected end of file" error before)
スクリーンショット 2025-06-14 10 09 37
  • no errors with server_hasdata.js as before
スクリーンショット 2025-06-14 21 33 10

@sgasho sgasho force-pushed the fix/no_error_with_empty_gzip branch 3 times, most recently from d9fe493 to bb750a2 Compare June 14, 2025 12:58
@sgasho sgasho force-pushed the fix/no_error_with_empty_gzip branch from bb750a2 to dedb17b Compare June 14, 2025 13:02
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few feedback for the code

@sgasho sgasho force-pushed the fix/no_error_with_empty_gzip branch from aebdbf5 to ccdb74d Compare June 14, 2025 15:44
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks LGTM!

cc @robjtede can you have a double check please?

@robjtede
Copy link
Member

robjtede commented Jun 15, 2025

I think it's important to note that this is a degenerate case, and as such may actually break some expectations of other consumers.

When we say "an empty gzip", we actually mean a real gzip file which decompresses to nothing.

What were allowing here is just "an empty file" and pretending that's a valid gzip, which it isn't:

$ touch a.gz
$ gunzip a.gz
gunzip: a.gz: unexpected end of file

Tools like curl do try to be more lenient when the degenerate cases are obvious, such as empty response payloads. I haven't looked up whether it does this check itself or delegates to its decompression library. (I know in Actix Web, we have similar optimizations before calling into to our [de]compression libs.)

I'm torn with async-compression, as a thin wrapper around these libraries, whether we should be putting any extra heuristics in place for this detection (is there any precedent for us doing this?) or whether the responsibility to implement this improvement should be given to Deno's client.

@NobodyXu If we want to accept that responsibility for all our library's consumers then this impl LGTM.

@NobodyXu
Copy link
Collaborator

Hmmm...I wonder if other algorithm has the same behavior, or is that just gzip?

for this PR, maybe we can make the behavior opt-in, if it is hard to implement this behavior outside async-compression @sgasho @robjtede ?

@robjtede
Copy link
Member

My personal view is that it's a bug in Deno that lead to this request, it shouldn't be trying to decompress something it knows is empty.

@NobodyXu
Copy link
Collaborator

Yeah, looking at the original issue seems like deno knows the body is empty before passing stuff to async-compression

@sgasho
Copy link
Author

sgasho commented Jun 17, 2025

Summary so far

The choices are

  • close this PR
  • make error message more detailed
  • make this feature as an opt-in
  • else

@robjtede
Copy link
Member

I was kinda waiting for someone on the Deno team to comment on that issue. It's nicer if this is a conversation rather than just saying "not our problem" immediately. Thankfully this is an edge case so it's not urgent to work out.

@robjtede robjtede marked this pull request as draft July 13, 2025 09:41
@robjtede robjtede added the A-semver-patch bug fixes label Jul 13, 2025
@robjtede
Copy link
Member

drafting til we hear from Deno team on that issue, i'm subscribed to it so will see updates when they happen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants