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

httpServer.close() not working correctly with slow Reader responses #1089

Closed
jakajancar opened this issue Jul 6, 2020 · 4 comments
Closed

Comments

@jakajancar
Copy link

This is the example from the website, with 2 modifications:

  1. s.close() is scheduled after 5 seconds
  2. The response body is a slow Reader
import { serve } from "https://deno.land/[email protected]/http/server.ts";
const s = serve({ port: 8000 });
console.log("http://localhost:8000/");
setTimeout(() => { s.close() }, 5000)
for await (const req of s) {
  req.respond({ body: {
    // Drips an 'x' every second
    async read(p: Uint8Array): Promise<number | null> {
        await new Promise(resolve => setTimeout(resolve, 1000))
        p.set([120])
        return 1
    }
  } });
}

Assuming you make an HTTP request in the first 5s, the connection will be closed immediately after close is called, but the loop will "notice" this a decade later, when the buffer fills and a write fails.

I think Server should be tracking ServerRequests, not only their Conn's, and on server.close() also fulfill req.done with a "server closed" error.

@ry
Copy link
Member

ry commented Jul 9, 2020

I'm not sure this is a bug as you have described it. server.close() should just stop accepting new connections and handling new requests (on previously opened pipelined connections)...

@jakajancar
Copy link
Author

Either makes sense to me:

  • stop accepting new requests (new connections and requests on existing connections), but finish existing ones (and resolve req.done of existing ones when they are done), or
  • kill everything, even if in the middle of e.g. sending a response (and reject req.done)

I don't really care about which, my only problem are promises that are left infinitely unresolved. I believe this is a bug.

@ry
Copy link
Member

ry commented Jul 9, 2020

my only problem are promises that are left infinitely unresolved. I believe this is a bug.

agree

@bartlomieju bartlomieju transferred this issue from denoland/deno Aug 3, 2021
@bartlomieju
Copy link
Member

std/http implementation was rewritten to leverage native HTTP bindings available in Deno (Deno.serveHttp). This error should no longer occur. I'm going to tentatively close this issue, please let me know if the problem persists.

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

No branches or pull requests

3 participants