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

Issue 6850067: code review 6850067: net/http: remove more garbage from chunk reading (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by bradfitz
Modified:
9 years, 7 months ago
CC:
dsymonds, dfc, gobot, golang-dev
Visibility:
Public.

Description

net/http: remove more garbage from chunk reading Noticed this while closing tabs. Yesterday I thought I could ignore this garbage and hope that a fix for issue 2205 handled it, but I just realized that's the opposite case, string->[]byte, whereas this is []byte->string. I'm having a hard time convincing myself that an Issue 2205-style fix with static analysis and faking a string header would be safe in all cases without violating the memory model (callee assumes frozen memory; are there non-racy ways it could keep being modified?)

Patch Set 1 #

Patch Set 2 : diff -r a26a8ada8f6e https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r a26a8ada8f6e https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 4 : diff -r 3d3b1e3f5268 https://go.googlecode.com/hg/ #

Total comments: 8

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

Patch Set 6 : diff -r 73c62d1bf08c https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+164 lines, -32 lines) Patch
M src/pkg/net/http/chunked.go View 1 2 3 5 chunks +30 lines, -16 lines 0 comments Download
M src/pkg/net/http/chunked_test.go View 1 2 3 4 2 chunks +52 lines, -0 lines 0 comments Download
M src/pkg/net/http/httputil/chunked.go View 1 2 3 5 chunks +30 lines, -16 lines 0 comments Download
M src/pkg/net/http/httputil/chunked_test.go View 1 2 3 4 2 chunks +52 lines, -0 lines 0 comments Download

Messages

Total messages: 15
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 7 months ago (2012-11-17 17:43:21 UTC) #1
dfc
Looks pretty reasonable to me. Do you have any benchmark data showing a drop in ...
9 years, 7 months ago (2012-11-19 08:30:53 UTC) #2
bradfitz
On Mon, Nov 19, 2012 at 12:30 AM, <dave@cheney.net> wrote: > Looks pretty reasonable to ...
9 years, 7 months ago (2012-11-19 16:32:00 UTC) #3
bradfitz
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-19 17:08:01 UTC) #4
gobot
R=dsymonds (assigned by bradfitz)
9 years, 7 months ago (2012-11-20 01:15:55 UTC) #5
dsymonds
https://codereview.appspot.com/6850067/diff/3006/src/pkg/net/http/chunked.go File src/pkg/net/http/chunked.go (right): https://codereview.appspot.com/6850067/diff/3006/src/pkg/net/http/chunked.go#newcode104 src/pkg/net/http/chunked.go:104: return trimTrailingWhitespace(p), nil return bytes.TrimRight(p, " \t\n\r"), nil and ...
9 years, 7 months ago (2012-11-20 01:24:22 UTC) #6
bradfitz
https://codereview.appspot.com/6850067/diff/3006/src/pkg/net/http/chunked.go File src/pkg/net/http/chunked.go (right): https://codereview.appspot.com/6850067/diff/3006/src/pkg/net/http/chunked.go#newcode104 src/pkg/net/http/chunked.go:104: return trimTrailingWhitespace(p), nil On 2012/11/20 01:24:22, dsymonds wrote: > ...
9 years, 7 months ago (2012-11-20 02:30:45 UTC) #7
bradfitz
Hello dsymonds@golang.org (cc: dave@cheney.net, gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-20 02:30:55 UTC) #8
dsymonds
LGTM https://codereview.appspot.com/6850067/diff/3006/src/pkg/net/http/chunked.go File src/pkg/net/http/chunked.go (right): https://codereview.appspot.com/6850067/diff/3006/src/pkg/net/http/chunked.go#newcode104 src/pkg/net/http/chunked.go:104: return trimTrailingWhitespace(p), nil On 2012/11/20 02:30:45, bradfitz wrote: ...
9 years, 7 months ago (2012-11-20 03:16:06 UTC) #9
bradfitz
On Mon, Nov 19, 2012 at 7:16 PM, <dsymonds@golang.org> wrote: > LGTM > > > ...
9 years, 7 months ago (2012-11-20 03:37:54 UTC) #10
bradfitz
*** Submitted as http://code.google.com/p/go/source/detail?r=14cfbda9e18a *** net/http: remove more garbage from chunk reading Noticed this while ...
9 years, 7 months ago (2012-11-20 03:50:46 UTC) #11
albert.strasheim
--- FAIL: TestChunkReaderAllocs-13 (0.00 seconds) chunked_test.go:65: 5 mallocs; want <= 1 FAIL in net/http and ...
9 years, 7 months ago (2012-11-20 07:46:23 UTC) #12
bradfitz
build.golang.org looks good... how's your machine different? even with GOMAXPROCS=13, aren't TestFoo funcs run serially? ...
9 years, 7 months ago (2012-11-20 07:59:08 UTC) #13
albert.strasheim
Hello On Tue, Nov 20, 2012 at 9:59 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > build.golang.org ...
9 years, 7 months ago (2012-11-20 09:02:26 UTC) #14
minux1
9 years, 7 months ago (2012-11-20 09:56:54 UTC) #15
On Tue, Nov 20, 2012 at 3:46 PM, <fullung@gmail.com> wrote:

> --- FAIL: TestChunkReaderAllocs-13 (0.00 seconds)
> chunked_test.go:65: 5 mallocs; want <= 1
> FAIL
>
> in net/http and net/http/httputil
>
Reproduced. sent https://codereview.appspot.com/6846081/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b