Issue 71230045: code review 71230045: net/rpc/jsonrpc: drop response on error
Descriptionnet/rpc/jsonrpc: drop response on error
Required by jsonrpc 1.0 spec
Fixes issue 7442.
Patch Set 1 #Patch Set 2 : diff -r 72b9500c86ce https://code.google.com/p/go/ #Patch Set 3 : diff -r 72b9500c86ce https://code.google.com/p/go/ #Patch Set 4 : diff -r 086daa1d2552 https://code.google.com/p/go/ #
MessagesTotal messages: 12
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
can you please add a test?
Sign in to reply to this message.
On 2014/03/05 20:03:45, rsc wrote: > can you please add a test? please take a look at the test, I don't know how to fix the issue with NewMockServerCodec
Sign in to reply to this message.
I've mailed out a much simpler test in https://codereview.appspot.com/69860056 Let me get that submitted and then you can update this CL to simply delete the t.Skip line. That work? Also, you don't appear to have submitted a CLA yet. See: http://golang.org/doc/contribute.html#copyright The individual form is at https://developers.google.com/open-source/cla/individual (some redirects are broken currently, if the first link leads you astray)
Sign in to reply to this message.
I've submitted the test. Please sync and update this CL to remove the t.Skip. Also let me know when you've submitted a CLA, so I can do that paperwork. On Wed, Mar 5, 2014 at 3:40 PM, <bradfitz@golang.org> wrote: > I've mailed out a much simpler test in > https://codereview.appspot.com/69860056 > > Let me get that submitted and then you can update this CL to simply > delete the t.Skip line. > > That work? > > Also, you don't appear to have submitted a CLA yet. See: > http://golang.org/doc/contribute.html#copyright > > The individual form is at > https://developers.google.com/open-source/cla/individual > > (some redirects are broken currently, if the first link leads you > astray) > > https://codereview.appspot.com/71230045/ >
Sign in to reply to this message.
On 2014/03/06 00:02:33, bradfitz wrote: > I've submitted the test. > > Please sync and update this CL to remove the t.Skip. Also let me know when > you've submitted a CLA, so I can do that paperwork. > > > > On Wed, Mar 5, 2014 at 3:40 PM, <mailto:bradfitz@golang.org> wrote: > > > I've mailed out a much simpler test in > > https://codereview.appspot.com/69860056 > > > > Let me get that submitted and then you can update this CL to simply > > delete the t.Skip line. > > > > That work? > > > > Also, you don't appear to have submitted a CLA yet. See: > > http://golang.org/doc/contribute.html#copyright > > > > The individual form is at > > https://developers.google.com/open-source/cla/individual > > > > (some redirects are broken currently, if the first link leads you > > astray) > > > > https://codereview.appspot.com/71230045/ > > Will do in a day or two, however I'm not sure if the test is correct as `func (t serverCodec) WriteResponse` writes to t.enc so I'm not sure the response will get written to the out variable defined in the test. Also I think it would be better to check the response contains correct value for the result (null) instead of checking it doesn't contain what we are supposedly passing in.
Sign in to reply to this message.
I verified the test works: it fails with nice output without the fix and passes with the fix. I can just submit your earlier version without a test if you're busy too. On Mar 6, 2014 3:29 AM, <yac.cz0@gmail.com> wrote: > On 2014/03/06 00:02:33, bradfitz wrote: > >> I've submitted the test. >> > > Please sync and update this CL to remove the t.Skip. Also let me know >> > when > >> you've submitted a CLA, so I can do that paperwork. >> > > > > On Wed, Mar 5, 2014 at 3:40 PM, <mailto:bradfitz@golang.org> wrote: >> > > > I've mailed out a much simpler test in >> > https://codereview.appspot.com/69860056 >> > >> > Let me get that submitted and then you can update this CL to simply >> > delete the t.Skip line. >> > >> > That work? >> > >> > Also, you don't appear to have submitted a CLA yet. See: >> > http://golang.org/doc/contribute.html#copyright >> > >> > The individual form is at >> > https://developers.google.com/open-source/cla/individual >> > >> > (some redirects are broken currently, if the first link leads you >> > astray) >> > >> > https://codereview.appspot.com/71230045/ >> > >> > > Will do in a day or two, however I'm not sure if the test is correct as > `func (t serverCodec) WriteResponse` writes to t.enc so I'm not sure the > response will get written to the out variable defined in the test. > > Also I think it would be better to check the response contains correct > value for the result (null) instead of checking it doesn't contain what > we are supposedly passing in. > > https://codereview.appspot.com/71230045/ >
Sign in to reply to this message.
Also, I notice you still haven't submitted a CLA. I will give this about 26 hours until I submit a fix myself. We're trying to close bugs. On Thu, Mar 6, 2014 at 6:43 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > I verified the test works: it fails with nice output without the fix and > passes with the fix. > > I can just submit your earlier version without a test if you're busy too. > On Mar 6, 2014 3:29 AM, <yac.cz0@gmail.com> wrote: > >> On 2014/03/06 00:02:33, bradfitz wrote: >> >>> I've submitted the test. >>> >> >> Please sync and update this CL to remove the t.Skip. Also let me know >>> >> when >> >>> you've submitted a CLA, so I can do that paperwork. >>> >> >> >> >> On Wed, Mar 5, 2014 at 3:40 PM, <mailto:bradfitz@golang.org> wrote: >>> >> >> > I've mailed out a much simpler test in >>> > https://codereview.appspot.com/69860056 >>> > >>> > Let me get that submitted and then you can update this CL to simply >>> > delete the t.Skip line. >>> > >>> > That work? >>> > >>> > Also, you don't appear to have submitted a CLA yet. See: >>> > http://golang.org/doc/contribute.html#copyright >>> > >>> > The individual form is at >>> > https://developers.google.com/open-source/cla/individual >>> > >>> > (some redirects are broken currently, if the first link leads you >>> > astray) >>> > >>> > https://codereview.appspot.com/71230045/ >>> > >>> >> >> Will do in a day or two, however I'm not sure if the test is correct as >> `func (t serverCodec) WriteResponse` writes to t.enc so I'm not sure the >> response will get written to the out variable defined in the test. >> >> Also I think it would be better to check the response contains correct >> value for the result (null) instead of checking it doesn't contain what >> we are supposedly passing in. >> >> https://codereview.appspot.com/71230045/ >> >
Sign in to reply to this message.
I've sent https://codereview.appspot.com/72570044 On Thu, Mar 6, 2014 at 10:49 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Also, I notice you still haven't submitted a CLA. I will give this about > 26 hours until I submit a fix myself. We're trying to close bugs. > > > > > On Thu, Mar 6, 2014 at 6:43 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> I verified the test works: it fails with nice output without the fix and >> passes with the fix. >> >> I can just submit your earlier version without a test if you're busy too. >> On Mar 6, 2014 3:29 AM, <yac.cz0@gmail.com> wrote: >> >>> On 2014/03/06 00:02:33, bradfitz wrote: >>> >>>> I've submitted the test. >>>> >>> >>> Please sync and update this CL to remove the t.Skip. Also let me know >>>> >>> when >>> >>>> you've submitted a CLA, so I can do that paperwork. >>>> >>> >>> >>> >>> On Wed, Mar 5, 2014 at 3:40 PM, <mailto:bradfitz@golang.org> wrote: >>>> >>> >>> > I've mailed out a much simpler test in >>>> > https://codereview.appspot.com/69860056 >>>> > >>>> > Let me get that submitted and then you can update this CL to simply >>>> > delete the t.Skip line. >>>> > >>>> > That work? >>>> > >>>> > Also, you don't appear to have submitted a CLA yet. See: >>>> > http://golang.org/doc/contribute.html#copyright >>>> > >>>> > The individual form is at >>>> > https://developers.google.com/open-source/cla/individual >>>> > >>>> > (some redirects are broken currently, if the first link leads you >>>> > astray) >>>> > >>>> > https://codereview.appspot.com/71230045/ >>>> > >>>> >>> >>> Will do in a day or two, however I'm not sure if the test is correct as >>> `func (t serverCodec) WriteResponse` writes to t.enc so I'm not sure the >>> response will get written to the out variable defined in the test. >>> >>> Also I think it would be better to check the response contains correct >>> value for the result (null) instead of checking it doesn't contain what >>> we are supposedly passing in. >>> >>> https://codereview.appspot.com/71230045/ >>> >> >
Sign in to reply to this message.
I sumbitted the CLA but I needed to do a fresh clone as I lost the original copy I had in ramfs and now the `go` binary doesn't build on the tip ( 19409:26aa53304a48 ) so I can't finish this, please push it yourself. On 2014/03/06 18:49:49, bradfitz wrote: > Also, I notice you still haven't submitted a CLA. I will give this about 26 > hours until I submit a fix myself. We're trying to close bugs. > > > > > On Thu, Mar 6, 2014 at 6:43 AM, Brad Fitzpatrick <mailto:bradfitz@golang.org>wrote: > > > I verified the test works: it fails with nice output without the fix and > > passes with the fix. > > > > I can just submit your earlier version without a test if you're busy too. > > On Mar 6, 2014 3:29 AM, <mailto:yac.cz0@gmail.com> wrote: > > > >> On 2014/03/06 00:02:33, bradfitz wrote: > >> > >>> I've submitted the test. > >>> > >> > >> Please sync and update this CL to remove the t.Skip. Also let me know > >>> > >> when > >> > >>> you've submitted a CLA, so I can do that paperwork. > >>> > >> > >> > >> > >> On Wed, Mar 5, 2014 at 3:40 PM, <mailto:bradfitz@golang.org> wrote: > >>> > >> > >> > I've mailed out a much simpler test in > >>> > https://codereview.appspot.com/69860056 > >>> > > >>> > Let me get that submitted and then you can update this CL to simply > >>> > delete the t.Skip line. > >>> > > >>> > That work? > >>> > > >>> > Also, you don't appear to have submitted a CLA yet. See: > >>> > http://golang.org/doc/contribute.html#copyright > >>> > > >>> > The individual form is at > >>> > https://developers.google.com/open-source/cla/individual > >>> > > >>> > (some redirects are broken currently, if the first link leads you > >>> > astray) > >>> > > >>> > https://codereview.appspot.com/71230045/ > >>> > > >>> > >> > >> Will do in a day or two, however I'm not sure if the test is correct as > >> `func (t serverCodec) WriteResponse` writes to t.enc so I'm not sure the > >> response will get written to the out variable defined in the test. > >> > >> Also I think it would be better to check the response contains correct > >> value for the result (null) instead of checking it doesn't contain what > >> we are supposedly passing in. > >> > >> https://codereview.appspot.com/71230045/ > >> > >
Sign in to reply to this message.
I still see no CLA on file, so I'm going to proceed with my CL. On Fri, Mar 7, 2014 at 2:09 PM, <yac.cz0@gmail.com> wrote: > I sumbitted the CLA but I needed to do a fresh clone as I lost the > original copy I had in ramfs and now the `go` binary doesn't build on > the tip ( 19409:26aa53304a48 ) so I can't finish this, please push it > yourself. > > > On 2014/03/06 18:49:49, bradfitz wrote: > >> Also, I notice you still haven't submitted a CLA. I will give this >> > about 26 > >> hours until I submit a fix myself. We're trying to close bugs. >> > > > > > On Thu, Mar 6, 2014 at 6:43 AM, Brad Fitzpatrick >> > <mailto:bradfitz@golang.org>wrote: > > > > I verified the test works: it fails with nice output without the fix >> > and > >> > passes with the fix. >> > >> > I can just submit your earlier version without a test if you're busy >> > too. > >> > On Mar 6, 2014 3:29 AM, <mailto:yac.cz0@gmail.com> wrote: >> > >> >> On 2014/03/06 00:02:33, bradfitz wrote: >> >> >> >>> I've submitted the test. >> >>> >> >> >> >> Please sync and update this CL to remove the t.Skip. Also let me >> > know > >> >>> >> >> when >> >> >> >>> you've submitted a CLA, so I can do that paperwork. >> >>> >> >> >> >> >> >> >> >> On Wed, Mar 5, 2014 at 3:40 PM, <mailto:bradfitz@golang.org> >> > wrote: > >> >>> >> >> >> >> > I've mailed out a much simpler test in >> >>> > https://codereview.appspot.com/69860056 >> >>> > >> >>> > Let me get that submitted and then you can update this CL to >> > simply > >> >>> > delete the t.Skip line. >> >>> > >> >>> > That work? >> >>> > >> >>> > Also, you don't appear to have submitted a CLA yet. See: >> >>> > http://golang.org/doc/contribute.html#copyright >> >>> > >> >>> > The individual form is at >> >>> > https://developers.google.com/open-source/cla/individual >> >>> > >> >>> > (some redirects are broken currently, if the first link leads >> > you > >> >>> > astray) >> >>> > >> >>> > https://codereview.appspot.com/71230045/ >> >>> > >> >>> >> >> >> >> Will do in a day or two, however I'm not sure if the test is >> > correct as > >> >> `func (t serverCodec) WriteResponse` writes to t.enc so I'm not >> > sure the > >> >> response will get written to the out variable defined in the test. >> >> >> >> Also I think it would be better to check the response contains >> > correct > >> >> value for the result (null) instead of checking it doesn't contain >> > what > >> >> we are supposedly passing in. >> >> >> >> https://codereview.appspot.com/71230045/ >> >> >> > >> > > https://codereview.appspot.com/71230045/ >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
