[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: ITS#3549, abandon/cancel/unbind



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, I believe Abandon and Unbind should always be allowed. 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...


 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