Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3954)

Issue 6640057: code review 6640057: net/http: fix race on bodyEOFSignal.isClosed (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by dfc
Modified:
1 year, 6 months ago
Reviewers:
CC:
dvyukov, bradfitz, golang-dev
Visibility:
Public.

Description

net/http: fix race on bodyEOFSignal.isClosed

Update issue 4191.

Fixes unreported race failure at
http://build.golang.org/log/61e43a328fb220801d3d5c88cd91916cfc5dc43c

Patch Set 1 #

Patch Set 2 : diff -r c3570e8ed478 https://code.google.com/p/go #

Patch Set 3 : diff -r c3570e8ed478 https://code.google.com/p/go #

Total comments: 6

Patch Set 4 : diff -r c3570e8ed478 https://code.google.com/p/go #

Patch Set 5 : diff -r 18dffd0c07b2 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r 18dffd0c07b2 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -4 lines) Patch
M src/pkg/net/http/transport.go View 1 2 3 4 chunks +13 lines, -4 lines 0 comments Download

Messages

Total messages: 11
dfc
Hello dvyukov@google.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
1 year, 6 months ago #1
bradfitz
The more concerning thing is that Close was called on two goroutines to begin with. ...
1 year, 6 months ago #2
bradfitz
LGTM okay, I looked. I'm happy with this. It's just as reasonable for line 575 ...
1 year, 6 months ago #3
dvyukov
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go File src/pkg/net/http/transport.go (right): https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837 src/pkg/net/http/transport.go:837: // already closed Is it OK that a goroutine ...
1 year, 6 months ago #4
dfc
Thank you for your comments. https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go File src/pkg/net/http/transport.go (right): https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820 src/pkg/net/http/transport.go:820: isClosed uint32 // atomic ...
1 year, 6 months ago #5
dfc
Hello dvyukov@google.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
1 year, 6 months ago #6
bradfitz
But I think Dmitry means that the loser still should wait for the winner's function ...
1 year, 6 months ago #7
dvyukov
Yes,that's what I mean. But I don't know whether its required. On Oct 11, 2012 ...
1 year, 6 months ago #8
bradfitz
I just looked. Doesn't seem to matter. If the user goroutine calls Close() first and ...
1 year, 6 months ago #9
dfc
Awesome. Thanks for the confirmation. On 12 Oct 2012 03:40, "Brad Fitzpatrick" <bradfitz@golang.org> wrote: > ...
1 year, 6 months ago #10
dfc
1 year, 6 months ago #11
*** Submitted as http://code.google.com/p/go/source/detail?r=cb38e940c38a ***

net/http: fix race on bodyEOFSignal.isClosed

Update issue 4191.

Fixes unreported race failure at
http://build.golang.org/log/61e43a328fb220801d3d5c88cd91916cfc5dc43c

R=dvyukov, bradfitz
CC=golang-dev
http://codereview.appspot.com/6640057
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5