> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan_at_visualsvn.com]
> Sent: donderdag 9 januari 2014 10:18
> To: Bert Huijben
> Cc: Subversion Development
> Subject: Re: svn commit: r1555716 -
> /subversion/trunk/subversion/libsvn_ra_serf/update.c
>
> On 7 January 2014 18:29, Bert Huijben <bert_at_qqmail.nl> wrote:
> > On 7 January 2014 12:42, Bert Huijben <bert_at_qqmail.nl> wrote:
> >> The current code works in both scenarios (tested via buffersizes of just a
> >> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
> >> is worth all the trouble. The file is null until something is spilled and
> >> from that point onwards everything is in the file (until the spill is
> >> empty).
> >>
> > Few bytes is bad test because memory isn't used at all in this case.
> >
> > I've tested and code doesn't work as expected. > I used 128, 256 but didn't
> get a problem. The same values might also hit a
> > corner case in the spill buffer. Will look at it in a few minutes.
> >
> Hi Bert.
>
> I see you already removed this optimization in r1556343. Thanks.
>
> But it seems there is another problem with using spillbuf for request
> bodies: create_update_report_body() callback can be called multiple
> times if request need to be resend because of authentication challenge
> or KeepAlive limit. In this case you will get empty request body on
> second invocation, because all spillbuf is already read.
>
> I see two options how to fix this:
> 1. Do not use spillbuf for request bodies. Use custom implementation
> based on stringbuf and temporary file if needed.
> 2. Implement svn_spillbuf__rewind() or something.
Wow... good catch. I'm surprised that we only find this now and not much shorter after I committed this. (Shouldn't we have tests for a simple authenticated scenario... or... well maybe the OPTIONS pipelining handles the simple cases?)
Instead of a rewind we could also use something like a 'peek' feature here (like on the serf buckets), which would keep us closer to the original intent.
Bert
Received on 2014-01-09 13:50:01 CET