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.

Sign up for GitHub

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

Jump to bottom

fix(ext/flash): graceful server startup/shutdown #16383

Merged
merged 13 commits into from
Nov 11, 2022
Merged

fix(ext/flash): graceful server startup/shutdown #16383

merged 13 commits into from
Nov 11, 2022

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Oct 21, 2022 •
edited
Loading

Fixes #16267

@littledivy littledivy marked this pull request as draft October 21, 2022 16:32
@magurotuna
Copy link
Member

magurotuna commented Nov 4, 2022 •
edited
Loading

I added some code to this patch. This includes:

benchmark comparison:

benchmark main (61fbfab) this PR (a9449f4)
deno_http_flash.js 72333.06 108153.61
deno_http_flash_spawn.js 113796.06 156837.42
deno_http_flash_ops.js 150221.73 161188.02
deno_http_flash_post_bin.js 6866.84 8258.77
deno_flash_send_file.js 15884.66 19876.11
deno_http_flash_ops_spawn.js 123948.80 170054.84
deno_flash_hono_router.js 59312.80 77773.57
deno_reactdom_ssr_flash.jsx 227.46 227.44

@littledivy littledivy marked this pull request as ready for review November 6, 2022 15:55
@littledivy littledivy changed the title [wip] fix(ext/flash): graceful server startup/shutdown [wip] Nov 6, 2022
@littledivy littledivy changed the title [wip] fix(ext/flash): graceful server startup/shutdown Nov 6, 2022
@magurotuna magurotuna mentioned this pull request Nov 8, 2022
Closed
@magurotuna magurotuna requested review from bartlomieju, billywhizz and piscisaureus and removed request for bartlomieju November 8, 2022 09:48
piscisaureus
piscisaureus approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think LGTM, but I'm not super familiar with this system.
@littledivy Can you review and land if you approve?

@littledivy
Copy link
Member Author

littledivy commented Nov 11, 2022 •
edited
Loading

Looks good. Thanks @magurotuna

@littledivy littledivy merged commit ff92feb into denoland:main Nov 11, 2022
@kt3k kt3k mentioned this pull request Nov 12, 2022
Closed
kt3k added a commit to kt3k/deno that referenced this pull request Nov 12, 2022
@kt3k
Revert "fix(ext/flash): graceful server startup/shutdown (denoland#16383
7f75820 
)"

This reverts commit ff92feb.
@kt3k kt3k mentioned this pull request Nov 12, 2022
Merged
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Nov 12, 2022
@littledivy @magurotuna @bartlomieju
fix(ext/flash): graceful server startup/shutdown (denoland#16383)
f47e1f9 
Fixes denoland#16267

Co-authored-by: Yusuke Tanaka <[email protected]>
kt3k added a commit that referenced this pull request Nov 13, 2022
@kt3k
fix(ext/flash): revert #16383 (graceful server startup/shutdown) (#16610
336e96a 
)

#16383 made some of Node compat test cases flaky in deno_std (and when
it fails it causes segfaults).

See denoland/std#2882 for details
magurotuna added a commit to magurotuna/deno that referenced this pull request Nov 13, 2022
@magurotuna
Revert "fix(ext/flash): revert denoland#16383 (graceful server startu…
6b934ee 
…p/shutdown) (denoland#16610)"

This reverts commit 336e96a.
@magurotuna magurotuna mentioned this pull request Nov 14, 2022
Merged
bartlomieju pushed a commit that referenced this pull request Nov 24, 2022
@magurotuna
fix(ext/flash): graceful server startup/shutdown with unsettled promi…
fd023cf 
…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.
bartlomieju pushed a commit that referenced this pull request Nov 24, 2022
@magurotuna @bartlomieju
fix(ext/flash): graceful server startup/shutdown with unsettled promi…
31abacb 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piscisaureus piscisaureus piscisaureus approved these changes

@billywhizz billywhizz Awaiting requested review from billywhizz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Used port is not freed when using Deno.run with --watch
3 participants
@littledivy @magurotuna @piscisaureus