Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource not release bug #500

Open
happysmile12321 opened this issue Mar 27, 2022 · 1 comment
Open

Resource not release bug #500

happysmile12321 opened this issue Mar 27, 2022 · 1 comment
Labels
bug Something isn't working needs investigation Something that needs to be looked at

Comments

@happysmile12321
Copy link

happysmile12321 commented Mar 27, 2022

in application.ts:

class Application->#handleRequest function:

async #handleRequest(
    request: ServerRequest,
    secure: boolean,
    state: RequestState,
  ): Promise<void> {
    const context = new Context(this, request, this.#getContextState(), secure);
    let resolve: () => void;
    const handlingPromise = new Promise<void>((res) => resolve = res);
    state.handling.add(handlingPromise);
    if (!state.closing && !state.closed) {
      try {
        await this.#getComposed()(context);
      } catch (err) {
        this.#handleError(context, err);
      }
    }
    if (context.respond === false) {
      context.response.destroy();
      resolve!();
      state.handling.delete(handlingPromise);
      return;
    }
    let closeResources = true; //this is true
    let response: Response;
    try {
      closeResources = false; // this is be modified to false,but no matter it enter catch or not catch,it be modified to false.
      response = await context.response.toDomResponse();
    } catch (err) {
      this.#handleError(context, err);
      response = await context.response.toDomResponse();
    }
    assert(response);
    try {
      await request.respond(response);
    } catch (err) {
      this.#handleError(context, err);
    } finally {
      context.response.destroy(closeResources);//in there , a lot of file descriptor not be closed by Deno.close(rid);
      resolve!();
      state.handling.delete(handlingPromise);
      if (state.closing) {
        state.server.close();
        state.closed = true;
      }
    }
  }

check out above comments, see if there has this problems.

@kitsonk kitsonk added bug Something isn't working needs investigation Something that needs to be looked at labels Mar 27, 2022
@NeKzor
Copy link

NeKzor commented Nov 14, 2023

I just ran into this bug today. This is not great when you try to build a reliable system just to figure out that things start to break in your production environment. A temporary workaround is to increase the nofiles ulimit. File descriptor exhaustion is a security issue. I don't think anyone would want to run into a DoS attack and a data loss.

The code that sets closeResouces to trueand then to false obviously does not make any sense. Looks like it was overlooked in the commit 5ad9c82#diff-ff7224ffde62f111cc1f7212d7e7da46d90d7258a99ca13c0db39857fbb19a03L390-L394. But one question still remains: When do you ever not close resources? Any insights on this? @kitsonk

Here are the steps to reproduce this:

1.) Create a large file of size MAXBUFFER_DEFAULT

deno eval 'Deno.writeTextFileSync("payload.txt", "A".repeat(1_048_576))'

2.) Create a issue_500.ts file and start the server deno run -A issue_500.ts

import { Application } from "https://deno.land/x/oak@v12.6.1/mod.ts";

const app = new Application();

app.use(async (context, next) => {
  try {
    await context.send({
      root: Deno.cwd(),
      index: 'payload.txt',
    });
  } catch {
    await next();
  }
});

console.log('http://127.0.0.1:8000')

await app.listen({ port: 8000 });

3.) Observe FD leak after visiting http://127.0.0.1:8000

lsof -awp $(pgrep -f issue_500.ts) | grep payload.txt
deno    323386 nekz   16r      REG                8,5   1048576 18777583 /home/nekz/git/clone/oak/payload.txt

Deno version:

deno 1.38.1 (release, x86_64-unknown-linux-gnu)
v8 12.0.267.1
typescript 5.2.2

NeKzor added a commit to NeKzor/autorender that referenced this issue Jan 30, 2024
NeKzor added a commit to NeKzor/autorender that referenced this issue Feb 18, 2024
The previous changes fixed oakserver/oak#500 but it also polluted the
module scope which shadowed globalThis.Response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs investigation Something that needs to be looked at
Projects
None yet
Development

No branches or pull requests

3 participants