Issue 4435055: code review 4435055: http/spdy: new package (Closed)
|
Created:
13 years, 11 months ago by Ross Light Modified:
13 years, 10 months ago Reviewers:
CC:
bradfitz, agl1, rsc, golang-dev Visibility:
Public. |
Descriptionhttp/spdy: new package
Patch Set 1 #Patch Set 2 : diff -r 3589ecb41d80 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 3589ecb41d80 https://go.googlecode.com/hg/ #
Total comments: 22
Patch Set 4 : diff -r 2b3a54a4ad6c https://go.googlecode.com/hg/ #
Total comments: 20
Patch Set 5 : diff -r 2b3a54a4ad6c https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 6 : diff -r 2b3a54a4ad6c https://go.googlecode.com/hg/ #
MessagesTotal messages: 9
Hello bradfitzgo (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Random comments... (just bouncing all over. Not a thorough review yet.) Other Gophers will probably have feedback too. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go File src/pkg/http/spdy/protocol.go (right): http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:5: // The spdy package implements the SPDY protocol, draft 2. // The spdy package is an incomplete implementation of the SPDY protocol. // // The implementation follows draft 2 of the spec: // // https://sites.google.com/a/chromium.org/dev/spdy/spdy-protocol/spdy-protocol-... package spdy http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:21: type ControlFrameType uint16 document this at least briefly? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:25: TypeSynStream = 0x0001 I wonder if these should be typed. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:64: FlagFin = 0x01 typed? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:69: // A Frame is the low-level construct passed over a SPDY connection. A Frame is a framed message as sent between clients and servers. There are two types of frames: control frames (ControlFrame) and data frames (DataFrame). ... or something. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:72: GetFlags() FrameFlags Go code doesn't tend to use "Get" prefixes on things, especially not in the standard library. I wonder how much this interface actually buys you. Your frames will already be WriterTos. Do you need GetFlags() and GetData() when Flags and Data are already exported? Clients will know the type of thing they're operating on, right? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:87: nn, err := writeFrame(w, []interface{}{uint16(0x8002), f.Type, f.Flags}, f.Data) constant? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:109: const frameHeadSize = 5 // in bytes, excluding length field header size? Feel free to give it a full comment on the line(s) before. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:113: headBuffer := new(bytes.Buffer) var buf bytes.Buffer. no need to heap-allocate it, right? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:118: if headBuffer.Bytes()[0]&0x80 == 0 { what is 0x80? worth a constant? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:153: lengthField := make([]byte, 3) var lengthField [3]byte ? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:172: func writeFrame(w io.Writer, head []interface{}, data []byte) (n int, err os.Error) { i wonder if you this would be prettier if you just ditched the head parameter and made the callers write that out themselves. might be more readable. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:213: type hrSource struct { what is this? document. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:240: // A HeaderReader reads zlib-compressed headers from discontiguous sources. Not sure what this means. The fields of the struct aren't helping me understand. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:271: if err != nil { did you mean for the line above to say "err" instead of "_" ?
Sign in to reply to this message.
Brad: Here's the new version of the package. I reorganized the types so that there is only one type: the Frame struct. ControlFrame and DataFrame are now functions that create a Frame. The HeaderReader/HeaderWriter are present because the compressor/decompressor must be used serially, so these types will use multiple readers and writers over time. (It makes more sense with my server, but now I would need to rewrite the server because I didn't reflect the changes in BitBucket). Cheers, Ross http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go File src/pkg/http/spdy/protocol.go (right): http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:5: // The spdy package implements the SPDY protocol, draft 2. On 2011/04/19 00:22:27, bradfitzgo wrote: > // The spdy package is an incomplete implementation of the SPDY protocol. > // > // The implementation follows draft 2 of the spec: > // > // > https://sites.google.com/a/chromium.org/dev/spdy/spdy-protocol/spdy-protocol-... > package spdy Done. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:21: type ControlFrameType uint16 On 2011/04/19 00:22:27, bradfitzgo wrote: > document this at least briefly? Done. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:25: TypeSynStream = 0x0001 On 2011/04/19 00:22:27, bradfitzgo wrote: > I wonder if these should be typed. I think they probably should. Why are the status codes in http untyped? http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:64: FlagFin = 0x01 On 2011/04/19 00:22:27, bradfitzgo wrote: > typed? Done. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:118: if headBuffer.Bytes()[0]&0x80 == 0 { On 2011/04/19 00:22:27, bradfitzgo wrote: > what is 0x80? worth a constant? Just getting the most significant bit. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:213: type hrSource struct { On 2011/04/19 00:22:27, bradfitzgo wrote: > what is this? document. Done. http://codereview.appspot.com/4435055/diff/3005/src/pkg/http/spdy/protocol.go... src/pkg/http/spdy/protocol.go:271: if err != nil { On 2011/04/19 00:22:27, bradfitzgo wrote: > did you mean for the line above to say "err" instead of "_" ? That I did. Good catch.
Sign in to reply to this message.
This package is very public, but as a subpackage of http, I'm assuming that's intended. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go File src/pkg/http/spdy/protocol.go (right): http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:63: return fmt.Sprintf("Type(%#04x)", uint16(t)) fmt is a chunky package to pull in for just this. strconv could handle it. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:72: FlagClearPreviouslyPersistedSettings = 0x01 Since this flag applies to a different bitfield, it should probably be in its own const block, possible with a different type. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:90: (Version&0xff00)>>8 | 0x80, The & here is superfluous. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:91: (Version & 0x00ff), likewise http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:92: byte((t & 0xff00) >> 8), ditto http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:93: byte((t & 0x00ff) >> 0), ditto, and the shift. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:104: byte(streamId & 0x7f000000 >> 24), and here, expect for the first line, where it's probably clearer to mask after the shift. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:125: _, err = io.ReadFull(r, lengthField[:]) ReadFull could have some odd behaviour here. If it reads 0 bytes then it'll return EOF, but you really want ErrUnexpectedEOF. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:143: func (f Frame) Control() bool { IsControl? http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:183: byte(len(f.Data) & 0x00ff0000 >> 16), more superfluous & http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:201: const headerDictionary = `optionsgetheadpostputdeletetraceacceptaccept-charsetaccept-encodingaccept-languageauthorizationexpectfromhostif-modified-sinceif-matchif-none-matchif-rangeif-unmodifiedsincemax-forwardsproxy-authorizationrangerefererteuser-agent100101200201202203204205206300301302303304305306307400401402403404405406407408409410411412413414415416417500501502503504505accept-rangesageetaglocationproxy-authenticatepublicretry-afterservervarywarningwww-authenticateallowcontent-basecontent-encodingcache-controlconnectiondatetrailertransfer-encodingupgradeviawarningcontent-languagecontent-lengthcontent-locationcontent-md5content-rangecontent-typeetagexpireslast-modifiedset-cookieMondayTuesdayWednesdayThursdayFridaySaturdaySundayJanFebMarAprMayJunJulAugSepOctNovDecchunkedtext/htmlimage/pngimage/jpgimage/gifapplication/xmlapplication/xhtmltext/plainpublicmax-agecharset=iso-8859-1utf-8gzipdeflateHTTP/1.1statusversionurl` + "\x00" you use backticks, but it's a single line. Also, can not the \x00 be in the string? http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:205: type hrSource struct { I wonder if this could be replaced with a bytes.Buffer
Sign in to reply to this message.
Changes made. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go File src/pkg/http/spdy/protocol.go (right): http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:63: return fmt.Sprintf("Type(%#04x)", uint16(t)) On 2011/04/21 18:13:19, agl1 wrote: > fmt is a chunky package to pull in for just this. strconv could handle it. Done. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:72: FlagClearPreviouslyPersistedSettings = 0x01 On 2011/04/21 18:13:19, agl1 wrote: > Since this flag applies to a different bitfield, it should probably be in its > own const block, possible with a different type. Done. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:90: (Version&0xff00)>>8 | 0x80, On 2011/04/21 18:13:19, agl1 wrote: > The & here is superfluous. Currently, yes, but if the Version changes, this ensures that the code is still valid. The field is 15 bits long. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:93: byte((t & 0x00ff) >> 0), On 2011/04/21 18:13:19, agl1 wrote: > ditto, and the shift. I've seen other packages do this (a quick grep yields http://golang.org/src/pkg/compress/gzip/gzip.go#L61) http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:125: _, err = io.ReadFull(r, lengthField[:]) On 2011/04/21 18:13:19, agl1 wrote: > ReadFull could have some odd behaviour here. If it reads 0 bytes then it'll > return EOF, but you really want ErrUnexpectedEOF. I hadn't thought of that. Fixed. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:143: func (f Frame) Control() bool { On 2011/04/21 18:13:19, agl1 wrote: > IsControl? Done. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:201: const headerDictionary = `optionsgetheadpostputdeletetraceacceptaccept-charsetaccept-encodingaccept-languageauthorizationexpectfromhostif-modified-sinceif-matchif-none-matchif-rangeif-unmodifiedsincemax-forwardsproxy-authorizationrangerefererteuser-agent100101200201202203204205206300301302303304305306307400401402403404405406407408409410411412413414415416417500501502503504505accept-rangesageetaglocationproxy-authenticatepublicretry-afterservervarywarningwww-authenticateallowcontent-basecontent-encodingcache-controlconnectiondatetrailertransfer-encodingupgradeviawarningcontent-languagecontent-lengthcontent-locationcontent-md5content-rangecontent-typeetagexpireslast-modifiedset-cookieMondayTuesdayWednesdayThursdayFridaySaturdaySundayJanFebMarAprMayJunJulAugSepOctNovDecchunkedtext/htmlimage/pngimage/jpgimage/gifapplication/xmlapplication/xhtmltext/plainpublicmax-agecharset=iso-8859-1utf-8gzipdeflateHTTP/1.1statusversionurl` + "\x00" On 2011/04/21 18:13:19, agl1 wrote: > you use backticks, but it's a single line. Also, can not the \x00 be in the > string? Done. http://codereview.appspot.com/4435055/diff/3/src/pkg/http/spdy/protocol.go#ne... src/pkg/http/spdy/protocol.go:205: type hrSource struct { On 2011/04/21 18:13:19, agl1 wrote: > I wonder if this could be replaced with a bytes.Buffer I tried that in my first implementation, and zlib would start giving errors because the stream is simply flushed between frames, not closed (different sequences).
Sign in to reply to this message.
http://codereview.appspot.com/4435055/diff/12001/src/pkg/http/spdy/protocol.go File src/pkg/http/spdy/protocol.go (right): http://codereview.appspot.com/4435055/diff/12001/src/pkg/http/spdy/protocol.g... src/pkg/http/spdy/protocol.go:212: const headerDictionary = "optionsgetheadpostputdeletetraceacceptaccept-charsetaccept-encodingaccept-languageauthorizationexpectfromhostif-modified-sinceif-matchif-none-matchif-rangeif-unmodifiedsincemax-forwardsproxy-authorizationrangerefererteuser-agent100101200201202203204205206300301302303304305306307400401402403404405406407408409410411412413414415416417500501502503504505accept-rangesageetaglocationproxy-authenticatepublicretry-afterservervarywarningwww-authenticateallowcontent-basecontent-encodingcache-controlconnectiondatetrailertransfer-encodingupgradeviawarningcontent-languagecontent-lengthcontent-locationcontent-md5content-rangecontent-typeetagexpireslast-modifiedset-cookieMondayTuesdayWednesdayThursdayFridaySaturdaySundayJanFebMarAprMayJunJulAugSepOctNovDecchunkedtext/htmlimage/pngimage/jpgimage/gifapplication/xmlapplication/xhtmltext/plainpublicmax-agecharset=iso-8859-1utf-8gzipdeflateHTTP/1.1statusversionurl\x00" Please break this string onto multiple lines. const headerDictionary = "...." + "..." + "..." + etc
Sign in to reply to this message.
http://codereview.appspot.com/4435055/diff/12001/src/pkg/http/spdy/protocol.go File src/pkg/http/spdy/protocol.go (right): http://codereview.appspot.com/4435055/diff/12001/src/pkg/http/spdy/protocol.g... src/pkg/http/spdy/protocol.go:212: const headerDictionary = "optionsgetheadpostputdeletetraceacceptaccept-charsetaccept-encodingaccept-languageauthorizationexpectfromhostif-modified-sinceif-matchif-none-matchif-rangeif-unmodifiedsincemax-forwardsproxy-authorizationrangerefererteuser-agent100101200201202203204205206300301302303304305306307400401402403404405406407408409410411412413414415416417500501502503504505accept-rangesageetaglocationproxy-authenticatepublicretry-afterservervarywarningwww-authenticateallowcontent-basecontent-encodingcache-controlconnectiondatetrailertransfer-encodingupgradeviawarningcontent-languagecontent-lengthcontent-locationcontent-md5content-rangecontent-typeetagexpireslast-modifiedset-cookieMondayTuesdayWednesdayThursdayFridaySaturdaySundayJanFebMarAprMayJunJulAugSepOctNovDecchunkedtext/htmlimage/pngimage/jpgimage/gifapplication/xmlapplication/xhtmltext/plainpublicmax-agecharset=iso-8859-1utf-8gzipdeflateHTTP/1.1statusversionurl\x00" On 2011/04/21 19:19:39, rsc wrote: > Please break this string onto multiple lines. > > const headerDictionary = "...." + > "..." + > "..." + > etc > That seems much more reasonable. :)
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=12b47b318d2d *** http/spdy: new package R=bradfitz, agl1, rsc CC=golang-dev http://codereview.appspot.com/4435055 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
