Closed Bug 239474 Opened 16 years ago Closed 15 years ago

LDAP connections left open in address book

Categories

(MailNews Core :: LDAP Integration, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7final

People

(Reporter: mscott, Assigned: Bienvenu)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(5 files, 3 obsolete files)

Do an AB search against and LDAP directory.

Type in a couple letters real slowly like:
"AAB"

we create 3 ldap connections if you type them slowly enough.

If you close the AB the connections are not closed.

If you type the letters quickly, only one connections is created and it is
closed when you close the AB.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7final
does this also happen with autocomplete in a compose window? I vaguely remember
that being the initial complaint, but don't have that e-mail anymore.
no these problems are just with address book searching from the quick search bar.
If I do this, three connections are created, but two of them are closed
immediately, and the third is closed when the ab window is closed. I'm not able
to recreate a problem where the connections are left open.

Fixing the fact that three connections are created is going to be painful since
this is the way the code was designed to work, and it will need to be rewritten.

The basic problem is that SetAbView is called to do the autocomplete whenever
you pause for a couple seconds, so for every char you type, if you type slowly,
SetAbView is called. Each call closes the previous view (which closes the ldap
connection, in my tests) and then creates a new view, which opens a new ldap
connection, as a side effect of doing a get resource on an ldap uri with the
search term in it. In order to re-use the ldap connection, we'd need to re-use
the directory, and be able to re-initialize the directory, and re-use the
existing connection.

I'll look at how dramatic a change it would be...
Assignee: mscott → bienvenu
Status: ASSIGNED → NEW
Attached patch work in progress (obsolete) — Splinter Review
This shows the tack I'm taking - if a view is asked for a new search uri, we
try to re-use the current directory, but re-init it with a new uri. This allows
us to re-use the connection of the directory object. And the ldap directory now
knows how to handle getting re-init'ed with a new uri. There's at least one
problem with this patch - it eventually crashes with a bad uri from js - I
haven't tracked that down yet. I'm not happy about calling Init multiple times
- I might need to invent a new method for this, but that would involve changing
all the directory sub-classes.
Attached patch fix that works (obsolete) — Splinter Review
this patch seems to work fine. I'd like to see if it can be reduced a little,
especially the clearing out of the old cards code.
Attachment #145495 - Attachment is obsolete: true
Attached patch proposed fixSplinter Review
clean up duplicate code to clear existing view...
Attachment #145653 - Attachment is obsolete: true
Attachment #146538 - Flags: superreview?(mscott)
Blocks: 240476
Attachment #146538 - Flags: superreview?(mscott) → superreview+
fixed on trunk.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
fixed on the tbird 0.6 branch
this fix broke the address book in thunderbird. I don't think we changed
thunderbird JS files that were changed for seamonkey :)
If this fix goes into the 1.7 branch please also check in this thunderbird
patch otherwise AB entries don't show in tbird.
this change broke LDAP searching in thunderbird on the 0.6 branch and the trunk.
Backing it out fixes the regressions. I'm investigating. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
if this patch goes into 1.7 please make sure this second patch also goes in to
make sure LDAP searches from the AB for Thunderbird.
Attachment #147284 - Flags: superreview?(bienvenu)
Attachment #147284 - Flags: superreview?(bienvenu) → superreview+
Hmm, looks like LDAP searching is still broken in the contacts sidebar for
thunderbird with his bug fix. There must be another change we are missing. I'll
investigate.
If this bug fix gets checked into any other branch besides the trunk, make sure
this patch also goes with. It fixes the problem with broken ldap searching from
the contacts sidebar.
Sigh, this doesn't seem to be working - we're still spinning up new
connections...investigating.
Attached patch work in progress (obsolete) — Splinter Review
this patch makes it so we cache the ldap connection, and the message listener,
across queries. It's pretty close to working but needs to be cleaned up. I
don't understand completely why the nsAbLDAPDirectoryQuery has a hashtable of
listeners, since it was never getting re-used in the first place.  But, now
that I'm re-using it, I'm running into a bug that may need to be solved with
multiple listeners, I'm not sure...the problem is that if I have a query that's
returning results, and while results are getting returned, I type another
character such that the new query won't return any results, the last couple of
results from the previous query get displayed as results for the new query. I
think they're coming in *after* the second query gets started, but we're not
smart enough to know that they belong to the first query, since we're re-using
the message listener. I don't need to re-use the message listener, I guess, but
I do need the nsAbLDAPDirectoryQuery to hold a reference to the message
listener, so I can't use nsHashTable - maybe nsSupportsHashtable...
Attached patch proposed fixSplinter Review
this seems to work fine for me. However, I suspect that all the lock stuff can
go since this code is only ever accessed on the ui thread, if I understand
correctly.
Attachment #148767 - Attachment is obsolete: true
Attachment #148801 - Flags: superreview?(mscott)
Attachment #148801 - Flags: review?(dmose)
any idea why that lock code is in nsAbLdapdirectoryQuery? I'm pretty sure it's
only ever accessed from one thread.
Seth wonders if ldap replication runs on its own thread, and can cause directory
queries to get access by multiple threads. I doubt this, becuase I do't think
ldap replication would be using the same directory query object as ab
autocomplete, for example.
Attachment #148801 - Flags: superreview?(mscott) → superreview+
(In reply to comment #0)
Open the Addressbook, Click on "Advanced...". Select an LDAP server and type
some search terms. With every click on "Search" a new thread will be created.
Those threads will not be terminated. I've checked this on Windows and Linux
with Thunderbird 0.7. 
Btw. a similar thing happens in OpenOffice, each LDAP query will create a
"zombie" thread. Don't OpenOffice and Mozilla/Thunderbird share the same LDAP code?
Keywords: fixed-aviary1.0
Comment on attachment 148801 [details] [diff] [review]
proposed fix

r=sspitzer
Attachment #148801 - Flags: review?(dmose) → review+
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Product: MailNews → Core
This big is still a problem in Thunderbird 0.9 / OpenLDAP 2.0.x.

When I look at nsAbLDAPDirectoryQuery.cpp,
it seems that nsAbQueryLDAPMessageListener::DoSearch()
expects to search using an existing connection...
mSearchOperation->Init (mConnection, proxyListener, nsnull);

When I look at my slapd logs, it is clear that a new connection
is created every time nsAbQueryLDAPMessageListener::DoSearch()
is called. These zombie connection pile up until...
  - The user exist from Thunderbird.
  - The 'idletimeout' directive in slapd.conf kills them
  - The max number of file decriptors is reached and LDAP hangs,
    which happens if no 'idletimeout' is set.
Reopening as per comment 22.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Re-resolving.  This problem in comment 22 appears to have been a combination of
configuration issues and the fact that general LDAP connection sharing is not
yet implemented.
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
(An important correction to comment 22) 

I now see that mozilla DOES reuse ldap connection, but the AddressBook and
type-ahead/contacts-sidebar do not share connections, and each directory
uses a connection, so here is an worst-case example...

Users have directories defined for People and Groups, Customers and Partners.
Theses are all branches on the same LDAP server. The People directory could
be used for type-ahead (for example).

Launch Thunderbird and open a new message
- type 'joe' ........................... connection-1
- type 'tom' ........................... connection-1
Click Contacts, select the Groups Directory
- type 'accounting' .................... connection-2  
Select Partners (from the dropdown)
- type 'acme' .......................... connection-3
Select Customers (from the dropdown)
- type 'bigco' ......................... connection-4
Open the AddressBook, select Partners
- type 'midco' ......................... connection-5
Select Customers (from the dropdown)
- type 'smallco' ....................... connection-6
Select Groups (from the dropdown)
- type 'legal' ......................... connection-7
- type 'admins' ........................ connection-7
Select People (from the dropdown)
- type 'john' .......................... connection-8

These connection will persist until Thunderbird is closed.

You Linux server will suppost 1024 file descriptors per thread (by default),
so the main OpenLDAP process can supposrt less that 128 users that use
LDAP in this way.

File descriptors are also used by pam for uid lookups by incoming email,
and POP3 users authenticate (every few minutes). IMAP users have a session,
so fewer active connections are required. Fortunately, these connections 
from pam are closed right away.

-------------

To address this issue, you could set an idletimeout (in slapd.conf),
but this could create issues for Thunderbird when connection are lost.

You may want to increase the amount of file descriptors available to slapd.
To do this, add the following to "/etc/sysconfig/ldap".

SLAPD_OPTIONS="ulimit -n 2048"

Look in the "/etc/init.d/ldap" for for the exact location on your system.
Depends on: 252759
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.