Non-idempotent retry in ne_begin_request()

[Joe Orton]

On Mon, Jan 04, 2010 at 01:57:00PM +0100, Thomas F wrote:
> Hi Joe,
> 
> hmm, I'm not convinced yet, see below :-)
> 
> Joe Orton schrieb:
> > send_request() knows to only return NE_RETRY if it is using a persistent 
> > connection - see the "retry" variable.
> > The logic here ensures that a 
> > persistent connection is closed for a non-idempotent request; so 
> > send_request() should never return NE_RETRY there.
> 
> It won't return NE_RETRY due to timeout (the close-on-non-idempotent-req
> logic prevents that), but it still can NE_RETRY due to server failure,
> which means non-idempotent requests may be silently retried.

Can you spell out exactly where you think NE_RETRY is going to be 
returned on server failure for a non-idempotent request on a persistent 
connection?  For that case:

ne_begin_request():

 -> closes the connection for a non-idempotent request on a persistent 
connection, before calling send_request()
 -> then calls send_request()

send_request():

 -> calls open_connection(), which opens a new socket, and clears
    sess->persisted
 -> "retry" variable set to value of sess->persisted, i.e. zero
 -> the RETRY_RET() macro is passed "retry", i.e. zero, so will never
    evaluate to NE_RETRY
 -> the only places where send_request() or functions it calls should
    return NE_RETRY should be wrapped by RETRY_RET
 -> ergo, there should be no case where send_request() returns NE_RETRY

Am I missing something?

Regards, Joe