fix(ext/flash): graceful server startup/shutdown with unsettled promises in mind by magurotuna · Pull Request #16616 · denoland/deno

magurotuna · 2022-11-13T08:55:15Z

This PR resets the revert commit made by #16610, bringing back #16383 which attempts to fix the issue happening when we use the flash server with --watch option enabled.
Also, some code changes are made to pass the regression test added in #16610. These changes can be seen here: https://github.com/denoland/deno/compare/0f31256..99f9670

Closes #16699

…p/shutdown) (denoland#16610)" This reverts commit 336e96a.

… about to finish

magurotuna · 2022-11-14T05:12:07Z

ext/flash/lib.rs

+  // Now the flash thread is about to finish, but there may be some unsettled
+  // promises in the main thread that will use the socket. To make the socket
+  // alive longer enough, we move its ownership to the main thread.
+  for (tok, socket) in sockets {
+    if let Some(sender) = socket_senders.remove(&tok) {
+      // Do nothing if the receiver has already been dropped.
+      _ = sender.send(socket);
+    }
+  }
+


This is the key idea; to prevent sockets from being released when the flash server finishes, transmit them to each ServerContext. This ensures that unsettled promises that outlive the flash thread can use sockets later.

magurotuna · 2022-11-14T05:15:07Z

ext/flash/socket.rs

  pub parse_done: ParseStatus,
  pub buffer: UnsafeCell<Vec<u8>>,
  pub read_lock: Arc<Mutex<()>>,
+  pub _pinned: PhantomPinned,


This is not directly related to this PR, but I feel this field is a must so we can make sure that Stream is not moved when it's Pinned.

magurotuna · 2022-11-14T10:47:56Z

cli/tests/unit/flash_test.ts

+  let port: number;

  const server = Deno.serve({
    handler: (_req) => new Response("ok"),
    hostname: "localhost",
-    port: 4501,
-    signal: ac.signal,
-    onListen: onListen(listeningPromise),
-    onError: createOnErrorCb(ac),
+    port: 0,
+    signal: ac1.signal,
+    onListen: (addr) => {
+      port = addr.port;
+      listeningPromise.resolve();
+    },
+    onError: createOnErrorCb(ac1),


Using port number 4501 makes CI really flaky. On my local ubuntu machine I was not able to see this flakiness though.

kt3k

Confirmed this change doesn't cause the issue denoland/std#2882 anymore

LGTM

stale

kt3k · 2022-11-15T05:25:05Z

I found some test failures with net-related compat test cases.

If I run the following command in std, there seem many test failures:

path/to/target/debug/deno task node:test -- parallel/test-net

Look like this has a different problem? 🤔

Most failures show this error message:

error: AssertionError: Failed assertion: error: Uncaught BadResource: Bad resource ID
      this[kStreamBaseField]?.close();
                              ^
    at Object.close (deno:core/01_core.js:367:25)

magurotuna · 2022-11-15T07:56:52Z

ugh... Will dig into that. Minimum reproducible code would be appreciated.

magurotuna · 2022-11-15T08:29:40Z

What I noticed so far is the close op is called twice with the same resource ID. Still no idea the reason.

magurotuna · 2022-11-15T11:00:21Z

Super interesting, weird fact: if we build deno with --release option, the command path/to/target/release/deno task node:test -- parallel/test-net does SUCCEED. I confirmed this with several commits, including d8aafcc which is the latest commit in this branch. However, if we debug build, even 916598f (which is v1.28.0) does not get these tests to pass. I have no idea at all why this weird thing is happening.

magurotuna · 2022-11-22T07:44:00Z

Really flaky 🤷

bartlomieju · 2022-11-22T13:21:38Z

Yeah, I'm not able to repeat this flakiness locally either :/

magurotuna · 2022-11-23T03:08:24Z

Hmm, from my quick observation, there seems to be three patterns of test failures.

test release macos-12 timeout
- hopefully ci: increase timeout to 120 minutes #16764 alleviates this
bench release ubuntu-20.04-xl panic
- which can be seen at ef82211
test release(debug) ubuntu-20.04-xl fails at httpServerRejectsOnAddrInUse in flash_test.ts

Among these, only 3 is specific to this PR - other 2 failures happen on the main branch too.

magurotuna · 2022-11-23T06:06:16Z

d7ca7eb fell into the case 1 (macos timeout)

kt3k · 2022-11-23T07:39:49Z

Super interesting, weird fact: if we build deno with --release option, the command path/to/target/release/deno task node:test -- parallel/test-net does SUCCEED. I confirmed this with several commits, including d8aafcc which is the latest commit in this branch. However, if we debug build, even 916598f (which is v1.28.0) does not get these tests to pass. I have no idea at all why this weird thing is happening.

I also confirmed deno task node:test -- parallel/test-net doesn't have problem with --release build. I also found that debug build already have problem with these net related compat test cases in main. So those failures look irrelevant to this change (sorry for wasting your time!)

kt3k

LGTM

magurotuna · 2022-11-23T07:56:39Z

67aac4a fell into the case 3

magurotuna · 2022-11-23T14:30:35Z

@bartlomieju What do you think about the flakiness we are facing? I ran this failing test case locally line 10 times and saw no failure. Would it be okay to disable this test for the time being?

bartlomieju · 2022-11-23T14:47:09Z

@bartlomieju What do you think about the flakiness we are facing? I ran this failing test case locally line 10 times and saw no failure. Would it be okay to disable this test for the time being?

Yes, I think that's reasonable

magurotuna · 2022-11-24T11:02:15Z

@bartlomieju PTAL before actually getting it landed just in case.

bartlomieju

LGTM

…ses in mind (#16616) This PR resets the revert commit made by #16610, bringing back #16383 which attempts to fix the issue happening when we use the flash server with `--watch` option enabled. Also, some code changes are made to pass the regression test added in #16610.

…ed promises in mind (denoland#16616)" This reverts commit fd023cf.

@magurotuna

#16839) …ed promises in mind (#16616)" This reverts commit fd023cf. There are reports saying that Vite is often hanging in 1.28.2 and this is the only PR that changed something with HTTP server. I think we should hold off on trying to fix this and instead focus on #16787 CC @magurotuna

…2950) This reverts commit 5f5413d. This change was made in response to denoland/deno#16616. However, that change was reverted in denoland/deno#16839 due to it breaking Vite. Let's revert it here as well.

@magurotuna

#16839) …ed promises in mind (#16616)" This reverts commit fd023cf. There are reports saying that Vite is often hanging in 1.28.2 and this is the only PR that changed something with HTTP server. I think we should hold off on trying to fix this and instead focus on #16787 CC @magurotuna

magurotuna added 4 commits November 13, 2022 17:46

Revert "fix(ext/flash): revert denoland#16383 (graceful server startu…

6b934ee

…p/shutdown) (denoland#16610)" This reverts commit 336e96a.

add a regresstion test

0f31256

get it to pass the regression test

ab5f294

fix

ecfc696

magurotuna force-pushed the segv-flash branch from a189ab9 to ecfc696 Compare November 13, 2022 14:24

magurotuna added 4 commits November 13, 2022 23:53

ci again

415d437

impl !Unpin for Stream

662635f

send ownership of sockets to the main thread when the flash thread is…

21b188c

… about to finish

flatten imports

99f9670

magurotuna commented Nov 14, 2022

View reviewed changes

magurotuna added 4 commits November 14, 2022 14:16

Merge branch 'main' into segv-flash

cd2fe3c

pick up port number automatically

56ab0df

is it flaky?

d4516f3

is it flaky? 2

a994289

magurotuna commented Nov 14, 2022

View reviewed changes

magurotuna marked this pull request as ready for review November 14, 2022 10:48

magurotuna requested review from kt3k and littledivy and removed request for littledivy November 14, 2022 10:48

kt3k previously approved these changes Nov 14, 2022

View reviewed changes

magurotuna changed the title ~~fix(flash): wait for a response to be served before terminating the server~~ fix(ext/flash): graceful server startup/shutdown again Nov 14, 2022

magurotuna changed the title ~~fix(ext/flash): graceful server startup/shutdown again~~ fix(ext/flash): graceful server startup/shutdown with unsettled promises in mind Nov 14, 2022

Merge branch 'main' into segv-flash

d8aafcc

flaky? 3

712590a

bartlomieju and others added 2 commits November 22, 2022 23:14

Merge branch 'main' into segv-flash

9541730

fix

3f44b91

flaky?

d7ca7eb

magurotuna added 2 commits November 23, 2022 15:06

flaky? 2

33ee055

flaky? 3

67aac4a

kt3k approved these changes Nov 23, 2022

View reviewed changes

magurotuna added 2 commits November 24, 2022 15:01

ignore httpServerRejectsOnAddrInUse for now

0e29321

hang?

ce8d507

magurotuna requested a review from bartlomieju November 24, 2022 11:01

bartlomieju approved these changes Nov 24, 2022

View reviewed changes

bartlomieju merged commit fd023cf into denoland:main Nov 24, 2022

magurotuna deleted the segv-flash branch November 25, 2022 02:41

kt3k mentioned this pull request Nov 25, 2022

node compat test parallel/test-http-outgoing-finish-writable.js fails denoland/std#2944

Closed

bartlomieju added a commit to bartlomieju/deno that referenced this pull request Nov 26, 2022

Revert "fix(ext/flash): graceful server startup/shutdown with unsettl…

fcafd5e

…ed promises in mind (denoland#16616)" This reverts commit fd023cf.

bartlomieju mentioned this pull request Nov 26, 2022

Revert "fix(ext/flash): graceful server startup/shutdown with unsettl… #16839

Merged

cjihrig mentioned this pull request Nov 27, 2022

Revert "test(node/http): modify writable of ClientRequest (#2945)" denoland/std#2950

Merged

Conversation

magurotuna commented Nov 13, 2022 • edited by bartlomieju Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magurotuna Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magurotuna Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

magurotuna Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

kt3k left a comment

Choose a reason for hiding this comment

Uh oh!

kt3k commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magurotuna commented Nov 15, 2022

Uh oh!

magurotuna commented Nov 15, 2022

Uh oh!

magurotuna commented Nov 15, 2022

Uh oh!

magurotuna commented Nov 22, 2022

Uh oh!

bartlomieju commented Nov 22, 2022

Uh oh!

magurotuna commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magurotuna commented Nov 23, 2022

Uh oh!

kt3k commented Nov 23, 2022

Uh oh!

kt3k left a comment

Choose a reason for hiding this comment

Uh oh!

magurotuna commented Nov 23, 2022

Uh oh!

magurotuna commented Nov 23, 2022

Uh oh!

bartlomieju commented Nov 23, 2022

Uh oh!

magurotuna commented Nov 24, 2022

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

magurotuna commented Nov 13, 2022 •

edited by bartlomieju

Loading

magurotuna Nov 14, 2022 •

edited

Loading

kt3k commented Nov 15, 2022 •

edited

Loading

magurotuna commented Nov 23, 2022 •

edited

Loading