Re: ITS#3549, abandon/cancel/unbind

["Kurt D. Zeilenga" <Kurt@OpenLDAP.org>]

At 05:58 PM 6/17/2005, Howard Chu wrote:
>Kurt D. Zeilenga wrote:
>> At 03:33 PM 6/17/2005, Howard Chu wrote:
>>> I'm still questioning the current code (from connection.c rev
>>> 1.265) which removes some of Abandon's exemptions from the defer
>>> check. Since ultimately processing the Abandon will free up
>>> resources.  I think it should always go through, and we need to
>>> give the same treatment to Unbind.
>
>> Some of the restrictions were put in place for non-obvious reasons.
>> For instance, one of the reasons we defer Abandon during Bind is
>> because the Bind operation itself is not abandonable.  Of course,
>> clients which issue requests while Binding, especially non-anonymous
>> Binds, are quite broken.  Which is another reason I never paid much
>> attention to this case.
>
>Hm. RFC2251 doesn't say anything about this, although I see language in the ldapbis-protocol draft to this effect. Personally, while I think it's unlikely to occur often,

But if it does, we should ignore requests to abandon a Bind
(an most every non-search) operation.

>I believe Abandon and Unbind should always be allowed.

I agree with Unbind but not Abandon.  For instance, one should
never abandon an Unbind operation.

>With Unbind there's really nothing the server can do to protest; the client is going to close the connection whether we like it or not. For Abandon I think it's still something to consider - if the server takes too long to complete the Bind request, the client may want to give up and may try to do so in an orderly fashion.  I suppose they could just Unbind in this case, but they might have reasons for wanting to keep the connection open. I'm not going to worry about it...

The problem with allowing abandon of Bind operations
is that the client won't get any indication that the Bind
succeed or not, no idea whether the protocol version it requested
is supported, no idea whether it is authenticated or not, etc..
And, if its SASL bind, no idea if security layers have been
installed.

Abandon'ing a bind request is simply a real bad idea.  Best
just to ignore it (just as we need to ignore Abandon requests
to abandon Start TLS operations, and many other things).

I look at your patch and comment if needed later...

Kurt


>> If we place some restrictions on what kind of operations abandon can
>> abandon, then we give some more exemptions to Abandon. For instance,
>> if we restrict abandoning to only search requests, then things get
>> easier.  (Cancel can cancel more (but not all) kinds of operations.
>> But let's take on separately.)
>>
>> And, as far as thread restrictions, I disagree that an exemption
>> should be given.  Abandon will use additional resources to do its
>> work, and that's why the thread restriction should apply.
>>
>>> I'm not sure we can do anything about Cancel since we haven't
>>> parsed the request OID by the time we make the decision about
>>> deferring the operation. We could try parsing the OID earlier but
>>> that seems ugly. It also seems like a minor issue, and we can just
>>> leave things the way they are. It seems the main reason Cancel was
>>> implemented here was to allow terminating a Persistent Search
>>> operation, and for that purpose it will still work fine.
>>
>> The main purpose of Cancel is replace completely the function of
>> Abandon, which suffers from not having any indicator of success or
>> failure.  Of course, Cancel (and Abandon) are generally only useful
>> when the operation is log-lived, such as a subtree search, a subtree
>> rename, or some chained operation (in which case, the cancel should
>> be chained).
>
>To make Cancel as effective as Abandon, it needs to have the ability to abort a queued/pending operation. In the current code it cannot do this. I'm not sure I care about fixing that.
>
>Here's my proposed change to connection.c:
>diff -u -r1.310 connection.c
>--- connection.c        28 Apr 2005 16:36:08 -0000      1.310
>+++ connection.c        18 Jun 2005 00:41:55 -0000
>@@ -1474,16 +1474,32 @@
>        * already pending ops, let them go first.  Abandon operations
>        * get exceptions to some, but not all, cases.
>        */
>-       if (tag != LDAP_REQ_ABANDON && conn->c_conn_state == SLAP_C_CLOSING) {
>-               defer = "closing";
>-       } else if (tag != LDAP_REQ_ABANDON && conn->c_writewaiter) {
>-               defer = "awaiting write";
>-       } else if (conn->c_n_ops_executing >= connection_pool_max/2) {
>-               defer = "too many executing";
>-       } else if (conn->c_conn_state == SLAP_C_BINDING) {
>-               defer = "binding";
>-       } else if (tag != LDAP_REQ_ABANDON && conn->c_n_ops_pending) {
>-               defer = "pending operations";
>+       switch( tag ){
>+       default:
>+               /* Abandon and Unbind are exempt from these checks */
>+               if (conn->c_conn_state == SLAP_C_CLOSING) {
>+                       defer = "closing";
>+                       break;
>+               } else if (conn->c_writewaiter) {
>+                       defer = "awaiting write";
>+                       break;
>+               } else if (conn->c_n_ops_pending) {
>+                       defer = "pending operations";
>+                       break;
>+               }
>+               /* FALLTHRU */
>+       case LDAP_REQ_ABANDON:
>+               /* Unbind is exempt from these checks */
>+               if (conn->c_n_ops_executing >= connection_pool_max/2) {
>+                       defer = "too many executing";
>+                       break;
>+               } else if (conn->c_conn_state == SLAP_C_BINDING) {
>+                       defer = "binding";
>+                       break;
>+               }
>+               /* FALLTHRU */
>+       case LDAP_REQ_UNBIND:
>+               break;
>       }
>
>       if( defer ) {
>
>
>-- 
> -- Howard Chu
> Chief Architect, Symas Corp.       Director, Highland Sun
> http://www.symas.com               http://highlandsun.com/hyc
> Symas: Premier OpenSource Development and Support