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

net: Check for closing status when iterating Listener #3309

Merged
merged 2 commits into from Nov 9, 2019

Conversation

@nayeemrmn
Copy link
Contributor

nayeemrmn commented Nov 9, 2019

Fixes #3308.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

nayeemrmn commented Nov 9, 2019

import { serve } from "https://deno.land/std@v0.23.0/http/server.ts";

const body = new TextEncoder().encode("Hello World\n");
const s = serve(":8000");

const f = fetch("http://localhost:8000");

(async () => {
  console.log("http://localhost:8000/");
  for await (const req of s) {
    req.respond({ body });
    s.close()
  }
})();

f.then(x => console.log("fetched"));

It seems like @hayd's original example ^ needs a separate fix in std since the server iterator calls Listener::accept() directly. Should I do that here? Done.

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:listener-iterator branch from d3c1e4a to 637763d Nov 9, 2019
@ry
ry approved these changes Nov 9, 2019
Copy link
Collaborator

ry left a comment

LGTM - thanks for the fix!

@ry ry merged commit d586f11 into denoland:master Nov 9, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Nov 9, 2019

Should this work on latest master?

import { serve } from "https://deno.land/std@335e8bd3/http/server.ts";

const body = new TextEncoder().encode("Hello World\n");
const s = serve(":8000");

const f = fetch("http://localhost:8000");

(async () => {
  console.log("http://localhost:8000/");
  for await (const req of s) {
    await req.respond({ body });
    // only service the first req
    break
  }
  s.close();
})();

f.then(x => console.log("fetched"));

I'm still seeing the same error.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

nayeemrmn commented Nov 9, 2019

@hayd You're using deno on master, too?

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Nov 9, 2019

./target/release/deno after a cargo build --all

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Nov 9, 2019

@nayeemrmn Sorry, I made a mistake! It works! Fantastic!

(cargo build --all doesn't build release ❗️ )

@nayeemrmn nayeemrmn deleted the nayeemrmn:listener-iterator branch Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.