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

Ignore error of writing responses to aborted requests #546

Merged
merged 3 commits into from Jul 28, 2019

Conversation

@kt3k
Copy link
Contributor

commented Jul 27, 2019

This PR fixes the case described in #442. The test case reproduces the case given by @MarkTiedemann in the comment ( #442 (comment) ).

This change simply ignore the error when the writing failed.

I believe the corresponding part of go's std library is here.

fmt.Fprintf(c.rwc, "HTTP/1.1 "+publicErr+errorHeaders+publicErr)

This doesn't handle the error returned by fmt.Fprintf. So this patch follows the above.

closes #442

@zekth

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

This swallows only aborted connections? Or it may swallow also unexpected errors? Just wondering.

@kt3k kt3k force-pushed the kt3k:feature/closed-connection branch 7 times, most recently from 36664e9 to 8622d1d Jul 27, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@zekth
This swallows errors mainly when it throws during responding 400 responses to already errored requests. It shouldn't affect any successful requests.

@kt3k kt3k force-pushed the kt3k:feature/closed-connection branch 11 times, most recently from c917da5 to 26216bd Jul 27, 2019

@kt3k kt3k force-pushed the kt3k:feature/closed-connection branch from 26216bd to e4005d0 Jul 28, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

I skipped the test case on windows because it doesn't implement process.kill(...) and I couldn't find an alternative.

@@ -0,0 +1,9 @@
// Sleeps the given milliseconds and resolves.
export function sleep(ms: number): Promise<void> {

This comment has been minimized.

Copy link
@zekth

zekth Jul 28, 2019

Contributor

Nice addition. But we may also add this to _std? i often use it personnaly.

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 28, 2019

Author Contributor

I agree. This util is very useful. I moved the function to util/async.ts. (I also changed the name to delay because it sounds more general and node.js has delay module with almost the same purpose.)

@ry
Copy link
Contributor

left a comment

Looks good! Just one comment...

assert(serverIsRunning);
} finally {
// Stops the sever.
p.kill(2); // SIGINT

This comment has been minimized.

Copy link
@ry

ry Jul 28, 2019

Contributor

You can use Deno.Signal.SIGINT here

@@ -0,0 +1,9 @@
import { serve } from "../server.ts";

This comment has been minimized.

Copy link
@ry

ry Jul 28, 2019

Contributor

Can you add a comment describing the purpose of this

// This is an example of a server that responds with an empty body
@ry

ry approved these changes Jul 28, 2019

Copy link
Contributor

left a comment

LGTM

@ry ry merged commit 826deb1 into denoland:master Jul 28, 2019

5 checks passed

denoland.deno_std Build #20190728.10 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@kt3k kt3k deleted the kt3k:feature/closed-connection branch Jul 28, 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.