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
Merged

Ignore error of writing responses to aborted requests #546

merged 3 commits into from
Jul 28, 2019

Conversation

kt3k
Copy link
Member

@kt3k kt3k 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
Copy link
Contributor

zekth commented Jul 27, 2019

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

@kt3k
Copy link
Member Author

kt3k 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
Copy link
Member Author

kt3k 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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Looks good! Just one comment...

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

Choose a reason for hiding this comment

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

You can use Deno.Signal.SIGINT here

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

Choose a reason for hiding this comment

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

Can you add a comment describing the purpose of this

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

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 826deb1 into denoland:master Jul 28, 2019
@kt3k kt3k deleted the feature/closed-connection branch July 28, 2019 11:54
inverted-capital pushed a commit to dreamcatcher-tech/napps that referenced this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error "Cannot read property 'w' of null at iterateHttpRequests" on aborted connections
3 participants