Kill all pending accepts when TCP listener is closed #1517

kevinkassimo · 2019-01-14T01:21:47Z

Closes #1516 .

Keep track of the pending accept task and notify it when the corresponding TcpListener from ResourceTable is dropped.

Before: Hangs forever
After:

Other: Listener has been closed
    at DenoError (deno/js/errors.ts:19:5)
    at maybeError (deno/js/errors.ts:38:12)
    at handleAsyncMsgFromRust (deno/js/dispatch.ts:27:17)

and dies. This error could be caught.

This is a first-pass implementation (so might not be the best solution). I'm slightly worried about the performance implication of adding 2 ResourceTable locks.
(Benchmark from Travis seems no change though)

ry · 2019-01-14T03:57:53Z

src/tokio_util.rs

+ r.track_task(self.task_id);
+ return Ok(futures::prelude::Async::NotReady);
+ }
+ Err(e) => return Err(From::from(e)),


Probably should untrack here too?

ry · 2019-01-14T03:58:09Z

js/net_test.ts

+ err = e;
+ }
+ assert(!!err);
+ assertEqual(err.message, "Listener has been closed");


Cool - good test case

ry · 2019-01-14T03:59:55Z

src/resources.rs

+ // we need to track these task so that when the listener is closed,
+ // pending tasks could be notified and die.
+ // The tasks are indexed, so they could be removed when accept completed.
+ TcpListener(tokio::net::TcpListener, HashMap<usize, futures::task::Task>),


It seems like this is something TcpListener should take care of? Is this working around a bug in tokio? Maybe we ought to report and link here?

Not so sure about this... we could probably ask them...
(Also I think our usage here is different from its most publicly available API, so there might be a miss...)

Would you mind opening an issue and just describing what's we're experiencing https://github.com/tokio-rs/tokio
Ideally link that issue in the source code here.
It's also unclear to me what the expected behavior is.

Submitted: tokio-rs/tokio#846
I'll add a comment linking to this once there are some basic feedback from the Tokio team (that this is indeed a problem)

So I believe based on the discussion there we might just save the task and notify ourselves...

There seems to be some problems with sccache on AppVeyor... On Travis it seems that it is rejecting more network listening requests (tried locally on my Mac machine, and seems it is the firewall that rejects the listen after reaching a threshold and instead shows a permission prompt that requires manual confirmation...) Nvm, found the issue

ry · 2019-01-14T04:02:08Z

src/resources.rs

+ // If TcpListener, we must kill all pending accepts!
+ if let Repr::TcpListener(l, m) = r.unwrap() {
+ // Drop first
+ std::mem::drop(l);


I don’t think this is necessary - it will be dropped with the above remove

Indeed... Forgot to remove this (was part of initial experiment...)

ry · 2019-01-14T04:02:59Z

src/tokio_util.rs

+use std::sync::atomic::Ordering;
+lazy_static! {
+ // Keep unique such that no collisions in TcpListener accept task map
+ static ref NEXT_ACCEPT_ID: AtomicUsize = AtomicUsize::new(0);


Can you use a HashSet instead and avoid this machinery?

It seems that futures::task::Task does not implement a few traits necessary for comparison in HashSet...

kevinkassimo · 2019-01-15T16:10:40Z

Per tokio-rs/tokio#846 (comment)
I believe we should throw an error when multiple concurrent accept on the same listener is called. With such enforcement, we could simply keep an Option instead of a HashMap for the current underlying task and only need to notify this single task on listener drop.

I'll go and implement this if this sounds reasonable

ry · 2019-01-15T16:19:44Z

@kevinkassimo Thanks for investigating this. Yes, one accept at a time seems like a reasonable constraint for a single thread of JS. I can imagine that we might want to share servers among isolates in the future and accept in multiple workers... but let's do that later. You should mention in a comment something about how this may need to be improved for the multi-worker accept situation.

kevinkassimo · 2019-01-15T21:39:30Z

Should be ready for one more round of review. (Added a test for multiple concurrent accepts)

js/net_test.ts

src/resources.rs

js/net_test.ts

src/resources.rs

src/tokio_util.rs

ry

LGTM - very technical fix - thanks for digging through it.

ry · 2019-01-17T22:10:41Z

@kevinkassimo FYI I've now seen one failure of this test on Windows. Not sure what's happening - it's potentially flaky.

test netConcurrentAccept_permW0N1E0R0 assertEqual failed. actual = Another accept task is ongoing expected = Listener has been closedC:/deno/js/deps/https/deno.land/x/std/testing/mod.ts:23:14
        throw new Error(msg);
Error: actual: Another accept task is ongoing expected: Listener has been closed
    at assertEqual (file:https:///C:/deno/js/deps/https/deno.land/x/std/testing/mod.ts:28:11)
    at listener.accept.catch.e (file:https:///C:/deno/js/net_test.ts:33:5)
127.0.0.1 - - [17/Jan/2019 22:07:43] "GET /tests/subdir/mt_video_mp2t.t3.ts HTTP/1.1" 200 -
127.0.0.1 - - [17/Jan/2019 22:07:43] "GET /tests/subdir/no_ext HTTP/1.1" 200 -
``

kevinkassimo · 2019-01-17T22:24:46Z

@ry It's a race condition I believe: basically the second accept somehow polls earlier than the first one by the scheduler.

…closed

…2224)

…ener is closed (denoland#2224)" Crashes while running wrk against js/deps/https/deno.land/std/http/http_bench.ts This reverts commit 972ac03.

…closed (#2224)" (#2239) Crashes while running wrk against js/deps/https/deno.land/std/http/http_bench.ts This reverts commit 972ac03.

…TCP listener is closed (denoland#2224)" (denoland#2239)" This reverts commit 1af02b4.

kevinkassimo force-pushed the tcp/close_accept branch 3 times, most recently from 368f758 to 0eb056e Compare January 14, 2019 03:27

kevinkassimo mentioned this pull request Jan 14, 2019

[WIP] IRC Server Implementation denoland/std#99

Closed

kevinkassimo force-pushed the tcp/close_accept branch from 0eb056e to beac39b Compare January 14, 2019 03:44

ry reviewed Jan 14, 2019

View reviewed changes

tyskelvin approved these changes Jan 14, 2019

View reviewed changes

kevinkassimo mentioned this pull request Jan 14, 2019

TcpListener does not notify non-ready tasks on drop tokio-rs/tokio#846

Closed

kevinkassimo added 2 commits January 15, 2019 08:22

Kill all pending accepts when TCP listener is closed

5d55995

Nit 1

aa3b4ee

kevinkassimo force-pushed the tcp/close_accept branch from ae51d18 to 07b575b Compare January 15, 2019 18:17

hayd mentioned this pull request Jan 15, 2019

Function to kill a running process #1494

Closed

kevinkassimo force-pushed the tcp/close_accept branch from 07b575b to 3e686e2 Compare January 15, 2019 21:27

ry reviewed Jan 15, 2019

View reviewed changes

js/net_test.ts Outdated Show resolved Hide resolved

hayd reviewed Jan 15, 2019

View reviewed changes

src/resources.rs Outdated Show resolved Hide resolved

kevinkassimo force-pushed the tcp/close_accept branch from 3e686e2 to de7b233 Compare January 15, 2019 22:20

ry reviewed Jan 15, 2019

View reviewed changes

js/net_test.ts Outdated Show resolved Hide resolved

js/net_test.ts Show resolved Hide resolved

src/resources.rs Show resolved Hide resolved

src/tokio_util.rs Outdated Show resolved Hide resolved

src/tokio_util.rs Outdated Show resolved Hide resolved

Switch to Option

9f405cc

kevinkassimo force-pushed the tcp/close_accept branch from de7b233 to 9f405cc Compare January 15, 2019 22:39

ry approved these changes Jan 16, 2019

View reviewed changes

ry merged commit 431e455 into denoland:master Jan 16, 2019

keroxp added a commit to keroxp/deno that referenced this pull request Apr 27, 2019

recover: denoland#1517 Kill all pending accepts when TCP listener is …

81675be

…closed

keroxp mentioned this pull request Apr 27, 2019

recover: #1517 Kill all pending accepts when TCP listener is closed #2224

Merged

ry pushed a commit that referenced this pull request Apr 27, 2019

recover: #1517 Kill all pending accepts when TCP listener is closed (#…

972ac03

…2224)

ry added a commit that referenced this pull request Apr 28, 2019

Revert "recover: #1517 Kill all pending accepts when TCP listener is …

1af02b4

…closed (#2224)" (#2239) Crashes while running wrk against js/deps/https/deno.land/std/http/http_bench.ts This reverts commit 972ac03.

keroxp added a commit to keroxp/deno that referenced this pull request May 2, 2019

Revert "Revert "recover: denoland#1517 Kill all pending accepts when …

a41fd5b

…TCP listener is closed (denoland#2224)" (denoland#2239)" This reverts commit 1af02b4.

ry pushed a commit to keroxp/deno that referenced this pull request May 2, 2019

Revert "Revert "recover: denoland#1517 Kill all pending accepts when …

58041a4

…TCP listener is closed (denoland#2224)" (denoland#2239)" This reverts commit 1af02b4.

ry pushed a commit to keroxp/deno that referenced this pull request May 3, 2019

Revert "Revert "recover: denoland#1517 Kill all pending accepts when …

f9ef009

…TCP listener is closed (denoland#2224)" (denoland#2239)" This reverts commit 1af02b4.

kevinkassimo deleted the tcp/close_accept branch December 27, 2019 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kill all pending accepts when TCP listener is closed #1517

Kill all pending accepts when TCP listener is closed #1517

kevinkassimo commented Jan 14, 2019 •

edited

Loading

ry Jan 14, 2019

kevinkassimo Jan 14, 2019

ry Jan 14, 2019

ry Jan 14, 2019

kevinkassimo Jan 14, 2019 •

edited

Loading

ry Jan 14, 2019

kevinkassimo Jan 14, 2019 •

edited

Loading

kevinkassimo Jan 15, 2019 •

edited

Loading

ry Jan 14, 2019

kevinkassimo Jan 14, 2019

ry Jan 14, 2019

kevinkassimo Jan 14, 2019 •

edited

Loading

kevinkassimo commented Jan 15, 2019 •

edited

Loading

ry commented Jan 15, 2019

kevinkassimo commented Jan 15, 2019

ry left a comment

ry commented Jan 17, 2019 •

edited

Loading

kevinkassimo commented Jan 17, 2019

Kill all pending accepts when TCP listener is closed #1517

Kill all pending accepts when TCP listener is closed #1517

Conversation

kevinkassimo commented Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

kevinkassimo Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

kevinkassimo commented Jan 15, 2019 • edited Loading

ry commented Jan 15, 2019

kevinkassimo commented Jan 15, 2019

ry left a comment

Choose a reason for hiding this comment

ry commented Jan 17, 2019 • edited Loading

kevinkassimo commented Jan 17, 2019

kevinkassimo commented Jan 14, 2019 •

edited

Loading

kevinkassimo Jan 14, 2019 •

edited

Loading

kevinkassimo Jan 14, 2019 •

edited

Loading

kevinkassimo Jan 15, 2019 •

edited

Loading

kevinkassimo Jan 14, 2019 •

edited

Loading

kevinkassimo commented Jan 15, 2019 •

edited

Loading

ry commented Jan 17, 2019 •

edited

Loading