inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

Docs: unclear how to pool connections #96

Closed elonen closed 1 year ago

elonen commented 1 year ago

I'm having trouble figuring out how to properly pool connections - mainly if is_closed() is sufficient and/or efficient way to check if a connection can still be used for issuing new queries (assuming suitable bind() of course), or if maybe a .drive() call should be done before trying to use an existing connection, or something. Examples all seem to be one-shot connections, too. Could you please add some clarification to the readme (or perhaps is_closed() function comment) on this?

inejge commented 1 year ago

I'm having trouble figuring out how to properly pool connections

The library deliberately has no built-in connection pooling facility, see e.g., #61. You must use an external pool adapter crate, or implement it yourself. The is_closed() method exists to help a connection pool to quickly determine if a connection is still valid. In the r2d2 crate, ManageConnection::has_broken() is meant to use this.

I suppose I could add a short note in the top-level README about this.

elonen commented 1 year ago

Thank you. The two main points I wanted to find out while reading the docs were:

1) Is is_closed() fast enough to call before every query (or does it in fact already make a query or something, and you'd be better of just retrying after getting some error variant).

2) Is it a complete test, or are there known internal connection states where you'd want to reconnect immediately instead of giving up after a query fails. I.e. despite !is_closed(), is it still recommended to do something like this:

// pseudocode
for retries in 1 .. 3 {
  ldap = if (ldap.is_closed()) { connect() } else { ldap};
  match ldap.search(...) {
     Ok(res) => { return res; }
     Err(e) => match *e.kind() {
       LdapError::ConnectedButNotReally => { ldap=None; continue; }  // <-- ?
       _ => { log("Query failed bad. Giving up."); break; }
     }
  }
}
inejge commented 1 year ago
  1. Yes.
  2. No (as documented in the update you can see in the commit that closed this issue.) Whether you'd want to do a round-trip check depends on the requirements of your application.