70422 - Implement the nsILDAPService interface

Status: VERIFIED FIXED

(Directory :: LDAP XPCOM SDK, enhancement, P1)

Description

[Leif Hedstrom]

We need to implement the LDAP Service manage objectclass. It will be used to
"pool" LDAP connections, making sure they are properly connected, and when
appropriate, disconnected. It depends heavily on nsLDAPServer objects for
getting LDAP server specific information (like host, port etc.).

Comment 1

[Leif Hedstrom]

Taking this bug...

Comment 2

[Leif Hedstrom]

Attachment: This is a patch required to get the new nsLDAPService stuff to work

Comment 3

[Leif Hedstrom]

Attachment: IDL file for the new LDAP Service interface

Comment 4

[Leif Hedstrom]

Attachment: New C++ file for the nsLDAPService class

Comment 5

[Leif Hedstrom]

Attachment: Include file for nsLDAPService

Comment 6

[Leif Hedstrom]

Attachment: Ignore that first patch, it was missing two files

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: reviewer comments

Comment 8

[Leif Hedstrom]

Attachment: 2nd iteration on nsLDAPService files (3 files concat)

Comment 9

[Leif Hedstrom]

Attachment: 3rd iteration of nsLDAPService files (3 files)

Comment 10

[David :Bienvenu]

some comments:

mTimestamp(0) - don't think this will compile on the Mac - you might need to use
those LL_* macros.

There doesn't seem to be much ref-counting going on, at least in your get
routines. Should they be add-refing?

You don't need to check for null before deleting:
 if (mServers) {
        delete mServers;
    }


could just be

delete mServers.

I'm sure Brendan must have a few comments :-)

Comment 11

[Leif Hedstrom]

David says:

>mTimestamp(0) - don't think this will compile on the Mac - you might need to
>use
>those LL_* macros.

Ok, I'll talk to some local Mac guru about it. I've used similar initializers in
other modules, which seemed to work on Mac.

>There doesn't seem to be much ref-counting going on, at least in your get
>routines. Should they be add-refing?

Do I need to add-ref in the "internal" GetServer, GetMessage and GetConnection
(part of the nsLDAPServiceEntry class) methods?

The "public" methods nsLDAPService::GetServer, AddServer and GetConnection does
add-ref properly.

>You don't need to check for null before deleting:
> if (mServers) {
>        delete mServers;
>    }

Ok.


Thanks!

-- Leif

Comment 12

[Leif Hedstrom]

Attachment: 4th version, nsLDAPService.cpp in single file.

Comment 13

[Leif Hedstrom]

Attachment: 4th version, nsLDAPService.h in single file

Comment 14

[Leif Hedstrom]

Attachment: nsILDAPService.idl in single file

Comment 15

[David :Bienvenu]

you should definitely try to build on the Mac re the PRTIme stuff. Better safe
than sorry.  I use something like  LL_I2L(m_startTime, 0); I hadn't heard of
LL_zero() but that looks good.

Re the ref-count stuff, you can do what you want internally but I wanted to make
sure you know what's going on with the ref-counting and ownership. It looks to
me (though I could be wrong) that the ldap service does not own the servers and
connections (i.e., doesn't hold a reference), so the client of the service has
to make sure they remove the servers and connections from the service before
deleting them or else the service will have dangling pointers. That's OK, as
long as it's understood.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

nsILDAPService.idl
------------------

* In 2 places, NS_ERROR_NOT_AVAILABLE should really be documented as
  "couldn't create connection thread", since it's coming from
  nsILDAPConnection.idl, and that's how it's documented there.


nsLDAPService.cpp
-----------------
* DeleteServer(): please file a bug on the renaming deleted entries
  thing, since this will probably be an issue once we start observing
  the preferences.

* lines 340, 488, 551, 640, 759, 761: why are any casts at all
  necessary on these lines?

* OnLDAPMessage(): according to the API, null messages are allowed,
  though their exact semantics are not yet defined.  I suspect we may
  use this for timeouts or errors.  At least mark this with an XXX so
  we come back and check for this once we define null message semantics.
 
* re Bienvenu's comment on the service owning connections and servers:
  I believe the service does correctly own servers, the AddRef/Release
  code for this is in nsLDAPService::AddServer/DeleteServer, but it
  looks like it doesn't own the connection, which looks to me like
  it's probably unintentional.  I suspect the code would be easier to
  read and understand if the AddRef and Release logic were pushed down
  into the nsLDAPServiceEntry XPCOM getters and setters themselves.

Other those minor things, it looks good!

Comment 17

[Leif Hedstrom]

David says:

> you should definitely try to build on the Mac re the PRTIme stuff. Better safe
> than sorry.  I use something like  LL_I2L(m_startTime, 0); I hadn't heard of
> LL_zero() but that looks good.

Yep, I've fixed that (thanks!) and it builds and run on Mac now as well as Win32
and Linux. (I'm still waiting to get a Mac build systems, so I have to bug
others to help me... :(

I have a new patch, fixes a bug in nsLDAPConnection which caused havoc in the
service, as well as this Mac stuff. I'll post it here asap.

> Re the ref-count stuff, you can do what you want internally but I wanted to
make
> sure you know what's going on with the ref-counting and ownership. It looks to
> me (though I could be wrong) that the ldap service does not own the servers
and
> connections (i.e., doesn't hold a reference), so the client of the service has

The Service creates the connection objects, and owns message objects, and it
holds a reference to the "server" object. The connection object and message
object are stored in internal nsHashArrays, storing nsCOMPtr's.

> to make sure they remove the servers and connections from the service before
> deleting them or else the service will have dangling pointers. That's OK, as

Yes, good point. However, I do want the service to keep a reference to the
nsLDAPServer objects (and I add-ref that), even if the user deletes it. The
nsLDAPService is a caching mechanism for servers, connections and the the bind
message.

Thanks again!

Comment 18

[Leif Hedstrom]

Attachment: The 3rd patch for stuff not directly in nsLDAPService

Comment 19

[Leif Hedstrom]

Attachment: Yet another nsLDAPService.cpp version, see comments

Comment 20

[Leif Hedstrom]

Attachment: Another version of nsILDAPService.idl

Comment 21

[Leif Hedstrom]

Ok, here's the status right now:

1. I've changed the stuff related to long-long's and Mac. It builds and run on
Mac now.

2. I've fixed the other changes request by dmose.

3. I've fixed a bug in nsLDAPConnection, which although not related to the
service directly, it caused some serious problems while testing my code. I hope
it's ok to include it in this patch, since it's so minor (we need to initialize
a couple of variables in the constructor).

So, a new patch, and new nsLDAPService.cpp and nsILDAPService.idl are attached,
as well as a new patch. nsLDAPService.h has not changed since the "4th" version.

Comment 22

[Brendan Eich [:brendan]]

Arbitrary/nonsensical control flow alert:

    if (!aServer) {
        return PR_FALSE;
    } else {
        mServer = aServer;
    }

    return PR_TRUE;

The else-after-return is a non-sequitur, and the final return and its only
predecessor statement, mServer = aServer;, should be indented the same (one
4-space unit).  This glitch recurs earlier, with if (!mLeases) as the condition,
and again earlier:

    now = PR_Now();
    if (LL_IS_ZERO(now)) {
        return PR_FALSE;
    } else {
        mTimestamp = now;
    }

But PR_Now will never return 0 unless your system's clock is misconfigured to
the beginning of the epoch.  Why should this method return false in only that
case? Why not just set mTimestamp and let the chips fall where they may?

There's code in the nsLDAPConnection.cpp patch that mixes four- and two-space
indentation (nit, I know; also, beware diff -u adds a leading space on these
lines):

   if (mPendingOperations) {
       delete mPendingOperations;
   }

More else-after-return nonsense:

        if (mServers->Exists(&hashKey)) {
            return NS_ERROR_FAILURE;
        } else {
 
and a leak on Init error right after that:

            // Create the new server entry, and add it into the hash table
            //
            entry = new nsLDAPServiceEntry;
            if (!entry || !entry->Init()) {
                NS_ERROR("nsLDAPService::AddServer: out of memory ");
                return NS_ERROR_OUT_OF_MEMORY;
            }

else-after-return yet again:

            if (!entry->SetServer(aServer)) {
                return NS_ERROR_FAILURE;
            } else {
                NS_ADDREF(aServer);
                mServers->Put(&hashKey, entry);
            }

from DeleteServer, more capricious control flow and indentation:

    entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey));
    if (entry) {
        if (entry->GetLeases() > 0) {
            return NS_ERROR_FAILURE;
        } else {
            entry->DeleteEntry();
        }
    } else {
        // There is no Server entry for this key
        //
        return NS_ERROR_FAILURE;
    }

    return NS_OK;

There's no reason to put the failure early return in the outer else clause. 
Invert the sense of the if condition and return failure in the then part,
unindenting the else part (which should not include else-after-return either,
before entry->DeleteEntry() is called).

The null _retval checks with NS_ERROR and error return backstop code seem like
overkill -- who might call you with such violations of the rules of XPCOM?  If
developers, make 'em eat assertions.

More tomorrow.

/be

Comment 23

[Leif Hedstrom]

Brendan wrote:

> Arbitrary/nonsensical control flow alert:
>
>    if (!aServer) {
>        return PR_FALSE;
>    } else {
>        mServer = aServer;
>    }

Ok, I've fixed all of those. The code looks better now, thanks.

> But PR_Now will never return 0 unless your system's clock is misconfigured to
>the beginning of the epoch.  Why should this method return false in only that
>case? Why not just set mTimestamp and let the chips fall where they may?

Ok, changed.


> There's code in the nsLDAPConnection.cpp patch that mixes four- and two-space
> indentation (nit, I know; also, beware diff -u adds a leading space on these
> lines):

Dunno why that was, I didn't do it. But I've changed it.


 
> and a leak on Init error right after that:

Ack, good catch, fixed.

> The null _retval checks with NS_ERROR and error return backstop code seem like
> overkill -- who might call you with such violations of the rules of XPCOM?  If
> developers, make 'em eat assertions.

Ok, I don't know what to do here... I don't really care either way, but some
people want to use this error checks, several (including shaver) do not. I don't
have a strong opinion on this matter, particularly since I'm a total XPCOM
newbie. But, I'd like our code to be consistent at least, and that's what I've
tried to do.

I'm leaving this as-is for now, until someone tells me that not checking _retval
for NULL is the way to go throughout the entire XPCOM code.

I'm attaching a new version on nsLDAPService.cpp. I've changed the prototype for
SetTimestamp() to be "void" as well, changing it in the .h file as well (of
course).


Thanks a lot for the reviews, much appreciated!

Comment 24

[Leif Hedstrom]

Attachment: nsLDAPService.cpp after BE's first review

Comment 25

[Mike Shaver (:shaver emeritus)]

Do you really need to lock in nsLDAPService::Init()?  Can it be called on
multiple threads, or race with other manipulations?  I would think not (a bad
race will leak mLock or crash autolocking on a null mLock, by my reading.

Probably better to just require that Init be called before any off-thread
accesses are possible, which will almost always be the case anyway: calling code
with Initialize immediately after construction, before passing it to anything
that could race (especially since Init isn't part of the XPCOM interface).

    PR_Lock(mLock);
    entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey));
    if (!entry) {
        PR_Unlock(mLock);

The PR_Lock/PR_Unlock logic in RequestConnection is pretty complex: would an
nsAutoLock, as used elsewhere, not make that simpler?  Alternatively, it appears
that you only need to hold mLock when you're accessing the hash tables (rename
it to mTableLock for clarity?), so it would seem that you could unlock
unconditionally after mServers->Get (manually or via a small scope for an
nsAutoLock) and be done with it.

In ReconnectConnection, you acquire mLock very early on, and don't release it
until you return, but, again, it only seems to be necessary to protect the hash
table access.  

Better to keep the critical section minimal:
{
    nsAutoLock lock(mLock);
    entry = NS_STATIC_CAST(..., mServers->Get(&hashKey));
}
if (!entry) ...

(And especially to avoid calling other methods with the lock held.  It's easy to
end up deadlocking if you're not quite careful, which is why ``lock only in leaf
functions'' is a good rule of thumb.)

    rv = EstablishConnection(entry);
    entry->SetRebinding(PR_TRUE);
    if (NS_FAILED(rv)) {
        entry->SetRebinding(PR_FALSE);
        return rv;
    }

You SetRebinding twice in the error case.  Better to move the PR_TRUE call to
after the error check, or even after the PushListener stanza, if PushListener
doesn't depend on the effects of SetRebinding(PR_TRUE).

Hmmm.  I'm actually starting to suspect that you're using mLock to serialize all
accesses to nsLDAPServiceEntries, because of the lengths you go to
OnLDAPMessage.  If this is the case, _please_ add documentation to that effect
in the headers and implementation of nsLDAPServiceEntry.  Also, do you need to
recheck any entry state after the listener's OnLDAPMessage returns, in case it
mutates it on you? (If it's not the case, then the unlocking logic in
OnLDAPMessage could be a lot simpler.)

    conn = do_CreateInstance("@mozilla.org/network/ldap-connection;1",
                             &rv);

Is the decoupling implied by this code in EstablishConnection really the case? 
That is, could you really interoperate with another implementation of
ldap-operation;1?  If not, you're probably best off using the CID or even just a
direct construction of the class.

Do you really need to list NS_ERROR_OUT_OF_MEMORY and NS_ERROR_NULL_POINTER in
all those method comments in nsILDAPService.idl?  Also, you should start all
your sentences with capitals, and tidy the error-code detail indentation for
requestConnection.

       For the time being, it is vital that you call this function
     * when you're done with a Connection, and that you do not keep any
     * copies of the Connection object

Do you really mean ``copies of the Connection object'' (does it provide a
|clone| method?) or something more like ``any dangling pointers to the provided
Connection object''?   (I'm a little nervous about this, because of the problems
with people having pointers to other interfaces of the same object, etc., but
dmose has explained the ownership requirements here, and I don't have a better
solution to offer at the moment.)

Is it worth putting a (DEBUG-only?) flag in the Connection objects to track when
they're in the post-releaseConnection invalid state, and assert if they're
manipulated (possibly including addref and release) in that state?

Sorry for the delay in reviewing this: I got behind on Friday and didn't recover
until now, because of some critical time management errors.  I hope you can
still get this stuff done in time.

Comment 26

[Leif Hedstrom]

Shaver wrote:

>Do you really need to lock in nsLDAPService::Init()?  Can it be called on
>multiple threads, or race with other manipulations?  I would think not (a bad

No, removed.

>    PR_Lock(mLock);
>    entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey));
>    if (!entry) {
>        PR_Unlock(mLock);

I don't think this is enough. My concern is if someone calls in to
RequestConnection() or ReconnectConnection() concurrently. We could then
potentially ending up with two connections to the LDAP server. The idea with the
service is that it's a caching mechanism, to reuse the same LDAP connection
multiple times.

I've added a comment to the IDL file regarding this. I might be overly careful
in the locking, I'll look into this and see what I can relax. I'm just scared
that the internal state of a cached connection gets messed up.

>The PR_Lock/PR_Unlock logic in RequestConnection is pretty complex: would an
>nsAutoLock, as used elsewhere, not make that simpler?  Alternatively, it
>appears
>that you only need to hold mLock when you're accessing the hash tables (rename
>it to mTableLock for clarity?), so it would seem that you could unlock
>unconditionally after mServers->Get (manually or via a small scope for an
>nsAutoLock) and be done with it.

Again, I don't think that's enough, but I'll look into it.

>You SetRebinding twice in the error case.  Better to move the PR_TRUE call to
>after the error check, or even after the PushListener stanza, if PushListener
>doesn't depend on the effects of SetRebinding(PR_TRUE).

Bug, fixed. :)

>Hmmm.  I'm actually starting to suspect that you're using mLock to serialize
>all
>accesses to nsLDAPServiceEntries, because of the lengths you go to
>OnLDAPMessage.  If this is the case, _please_ add documentation to that effect

Ok, added this to the IDL file:

 * Much of the code here is serialized, to assure that we don't
 * end up with multiple instances of the connection to a server.
 * The main goal of this service is to cache connections to an LDAP
 * server, and it's important that we actually reuse connections.
 * This is not a serious bottleneck, since the calls into the service
 * are done fairly infrequently.


Also added this to the .cpp file.


>in the headers and implementation of nsLDAPServiceEntry.  Also, do you need to
>recheck any entry state after the listener's OnLDAPMessage returns, in case it
>mutates it on you? (If it's not the case, then the unlocking logic in
>OnLDAPMessage could be a lot simpler.)

When calling an OnLDAPMessage() listener, it's quite likely that the listener
will call back into the service. That's why the locking here got complicated.
I've tested this a lot, and it was needed to avoid deadlock issues. I might be
on drugs here, I can't say I'm an expert on concurrency issues, so if you have
some specific suggestions how to make this simpler, please! :)


>    conn = do_CreateInstance("@mozilla.org/network/ldap-connection;1",
>                             &rv);
>
>Is the decoupling implied by this code in EstablishConnection really the case? 
>That is, could you really interoperate with another implementation of
>ldap-operation;1?  If not, you're probably best off using the CID or even just
a
>direct construction of the class.

Don't know the answer to this one. Dmose, any suggestions?


>Do you really need to list NS_ERROR_OUT_OF_MEMORY and NS_ERROR_NULL_POINTER in
>all those method comments in nsILDAPService.idl?  Also, you should start all
>your sentences with capitals, and tidy the error-code detail indentation for
>requestConnection.

This was done to be consistent with other LDAP XPCOM code. I don't have a strong
opinion either way, as long as we are consistent.

>Do you really mean ``copies of the Connection object'' (does it provide a
>|clone| method?) or something more like ``any dangling pointers to the provided
>Connection object''?   (I'm a little nervous about this, because of the
problems

Dangling points. Fixed!

>with people having pointers to other interfaces of the same object, etc., but
>dmose has explained the ownership requirements here, and I don't have a better
>solution to offer at the moment.)

It's a known issue, and we will try to find a better solution, making
ReleaseConnection() obsolete.


>Is it worth putting a (DEBUG-only?) flag in the Connection objects to track
>when
>they're in the post-releaseConnection invalid state, and assert if they're
>manipulated (possibly including addref and release) in that state?

Dunno, maybe. I didn't want to spend an horrendous amount of time on this issue,
since they goal is to get rid of the entire ReleaseConnection() jive. I'll look
into it though.


Thanks a lot for the review! I'll post an updated version of the files as soon
as I can. If you can look at the new files when they are available, that'd be
great.

Btw, did you notice any missing add-reffing as indicated by David? It's one of
my main concerns, particularly since I'm such a newbie to all this XPCOM stuff.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Regarding the use of contract-id here, I think it's ok, since, unlike most of
the rest of the XPCOM SDK, the service doesn't actually know any thing about the
guts of the other classes nor does it call into the LDAP C SDK directly.

brendan/shaver: the NULL-checking at interface boundaries and documenting of the
same is done throughout the XPCOM SDK code, not just here.  If you really think
this is important enough to get rid of, please open another bug against me and
I'll write the full essay describing why I think it's actually a good thing.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Oh yeah, once shaver and brendan's concerns are satisfied, r=[e-mail].

Comment 29

[Leif Hedstrom]

Attachment: nsLDAPService.cpp, after Shavers review

Comment 30

[Leif Hedstrom]

Attachment: nsLDAPService.h, after Shavers review

Comment 31

[Leif Hedstrom]

Attachment: nsILDAPService.idl, after Shavers review

Comment 32

[Leif Hedstrom]

Ok, posted three new files, addressing most of Shavers concerns. Please review
again?

Thanks,

-- Leif

Comment 33

[Mike Shaver (:shaver emeritus)]

If I read your new comment regarding serialization, it still sounds like you
only need to serialize hash table access, in order to make sure that two
additions of the same server's entry don't race and end up with duplicates.

What other operations need to be serialized to avoid duplicates?

The refcounting looks fine to me: the connection cache keeps references to the
cached objects, so you don't need to twiddle them for the internal methods in
question.

As far as the flag for use of released connections, I'm torn.  On the one hand,
I can see not wanting to burn the cycles on it, but on the other you're going to
considerable lengths to defend against relatively easy-to-diagnose API errors
like the passing of NULL, and yet you leave this case, which is harder for
callers to diagnose and a much more subtle API requirement.  I think the fact
that the ReleaseConnection stuff is supposed to go away (is there a bug on
that?) tips the balance in favour of just letting it slide, but I'm getting more
and more tempted to ask for dmose's treatise on NULL checking.

The only thing I'm still unsure about is the serialization; I really want to
understand the requirements before I stamp here, and it still looks to me like
just serializing the hash table accesses will be sufficient. (And I really
prefer to keep locking to minimal sections: see 76661 for an example of where
we've been burned by holding locks across too many calls.)

Comment 34

[Leif Hedstrom]

ok, let me look into this again then, see if I can relax the locks.

Comment 35

[Leif Hedstrom]

Shaver wrote:

>If I read your new comment regarding serialization, it still sounds like you
>only need to serialize hash table access, in order to make sure that two
>additions of the same server's entry don't race and end up with duplicates.
>
>What other operations need to be serialized to avoid duplicates?

I've made some changes to locking, but not what you are asking for. The problem
is this: I don't want the internal state of a cached connection to get messed
up. Or even worse, have two LDAP connections established concurrently, and then
have to drop one when "collision" is detected.

The reason there was serialization in RequestConnection()/ReconnectConnection()
was to avoid any such possiblity. I've spent all morning trying to figure out
how I can remove the locks here, without causing race conditions. The best I can
accomplish (with a lot of if() statements etc.) is a situation where it's still
possible to create extra connections to the LDAP server, which are then dropped
on the floor (delete) when the collision is detected. That just seems wrong to
me (but I might be on crack of course).

Another similarly poor alternative was to allow multiple LDAP connections to
happen, but only cache one of them (and let the other die when it's consumer
releases it).

Granted, this is fairly unlikely to happen, but the code was written to not let
it happen at all. Hence the serialization on the creation/reconnection of LDAP
connection objects.


>As far as the flag for use of released connections, I'm torn.  On the one hand,
>I can see not wanting to burn the cycles on it, but on the other you're going
to
>considerable lengths to defend against relatively easy-to-diagnose API errors

Well, I'm torn too... :) However, one of the main reasons to keep those checks
you despise is to keep consistent with the rest of LDAP XPCOM.


>callers to diagnose and a much more subtle API requirement.  I think the fact
>that the ReleaseConnection stuff is supposed to go away (is there a bug on
>that?) tips the balance in favour of just letting it slide, but I'm getting

Yep, there is a bug on that, bug 75954. It's also documented in the IDL, marked
to become depracated in the future (I'd make it a noop).

I need to talk to you about the serialization, I can't figure out how to do
without it. :( And, I'm looking forward to see you and dmose fight out the
battle of NULL pointer checks on the parameters in the API, it will be a most
excellent battle.

Thanks.

-- leif

Comment 36

[Leif Hedstrom]

Attachment: Minor locking improvements in the service (very minor)

Comment 37

[Mike Shaver (:shaver emeritus)]

Are all operations on connections meant to be serialized, then?  In other words,
is it possible for two threads to be playing with the same connection at once,
even after they've serially retrieved it from the LDAPService?  If so, you need
to lock on the |entry|, not just the service.

I chatted with dmose last night -- bugger called me at _home_! -- and read the
code some more, and I think my biggest concern can be met with this change:

  - Make the LDAPService more tightly coupled to the connection, either by use
    of a CID or just |new nsLDAPConnection|.  This will assuage my fears about
    an alternate implementation trying to call back into the LDAP service during
    initialization and deadlocking[*].

With that change, sr=shaver.  You don't have to push the locking down into the
entry for this checkin but please, I beg of you, write something in
directory/xpcom/doc/threading.html that talks about what operations are
serialized against what others, etc.

[*] Maybe we need nsReentrantLock?  Maybe a reader-writer lock is a useful tool
here?  I'll ponder it.

Now, back to resting.

Comment 38

[Leif Hedstrom]

>Are all operations on connections meant to be serialized, then?  In other
>words,
>is it possible for two threads to be playing with the same connection at once,
>even after they've serially retrieved it from the LDAPService?  If so, you need
>to lock on the |entry|, not just the service.

This I don't understand... The LDAP Service caches and manages nsLDAPConnection
and nsLDAPMessage objects. An nsLDAPConnection can be shared by multiple
threads, which is one reason why I go through this serialization process (to
make sure we don't step on each other).

>I chatted with dmose last night -- bugger called me at _home_! -- and read the
>code some more, and I think my biggest concern can be met with this change:

Sorry, dmose is a buttmunch. :-p


>  - Make the LDAPService more tightly coupled to the connection, either by use
>    of a CID or just |new nsLDAPConnection|.  This will assuage my fears about
>    an alternate implementation trying to call back into the LDAP service
>during
>    initialization and deadlocking[*].

I did this after you first mentioned it - see the attachment from 04/30/01. From
then on, I use 

   static NS_DEFINE_CID(kLDAPConnectionCID, NS_LDAPCONNECTION_CID);

and then

   conn = do_CreateInstance(kLDAPConnectionCID, &rv);


Is this not what you had in mind?

>With that change, sr=shaver.  You don't have to push the locking down into the
>entry for this checkin but please, I beg of you, write something in
>directory/xpcom/doc/threading.html that talks about what operations are
>serialized against what others, etc.

Ok, I'll do that. The serialization is primarily around RequestConnection() and
ReconnectConnection(), which is where the risk of ending up with extra
connections to the LDAP server is worse. The design is made where the "helper"
function EstablishConnection() is atomic, and it's used by those two methods
mentioned. If I push down the locking into the entry, the code would still work,
but I'm pretty sure there can be cases then where we establish a new LDAP
connection, which has to be dropped or used only once (which defeats the purpose
of the LDAP service).

>[*] Maybe we need nsReentrantLock?  Maybe a reader-writer lock is a useful tool
>here?  I'll ponder it.

>Now, back to resting.

Hope you feel better soon, and thanks again for looking into this.

Comment 39

[Leif Hedstrom]

I've created bug 78758, Reducing the serialization in nsLDAPService. I believe
shaver is right (*sigh*) and we can reduce a lot of the serialization in
RequestConnection() and ReconnectConnection().

Still hoping I can check this version in, it meets the criteria for Shavers SR=,
and it does reduce a number of locking issues compared to earlier versions.

Comment 40

[Brendan Eich [:brendan]]

All those getters and setters for nsLDAPServiceEntry data members should be
inline, I say.

What invariants is nsLDAPService::mLock supposed to protect?  The mServers hash
table including all of its entries' state, at least.  But why hold it all the
way across EstablishConnection?  It's bad to hold a lock (which might use SMP
competitive spinning for short waits) across a bunch of do_CreateInstance calls
-- and what about conn->Init(...) and operation->Init() do, never mind
operation->SimpleBind(0)?

It would be better to release the lock and protect the lifetime of the entry in
use, if not its state.  Or look it up again after releasing the lock and
establishing the connection, and if not found or found but of a new generation
(not the same entry we were using), cope somehow.

More in a bit.

/be

Comment 41

[Leif Hedstrom]

Attachment: Post-BE review, nsILDAPService.idl

Comment 42

[Leif Hedstrom]

Attachment: nsLDAPService.cpp, BE style

Comment 43

[Leif Hedstrom]

Attachment: nsLDAPService.h, maybe this will work...

Comment 44

[Brendan Eich [:brendan]]

> class nsLDAPService : public nsILDAPService, nsILDAPMessageListener

Don't you need a public before the second class from which nsLDAPService
inherits?

I worry you'll get out-of-line copies of GetMessage, etc. by not doing the
inlining directly in the nsLDAPServiceEntry class declaration.  Worth getting
right now?  Your call, if testing shows my worry is real.

The extra block in nsLDAPService::GetServer to scope the nsAutoLock is not
necessary any longer, if it ever was.  Nit, easy to fix but entirely up to you. 
Ditto GetConnection.

Should nsLDAPService::AddServer fail on collision, or just return NS_OK?  There
is no out param or return value by which the caller can tell that it added the
server, rather than finding one there (due to a race, if the caller checked;
else perhaps already there for a while!).  Why fail?

In RequestConnection, please fuse the big common tail of the final if-else:

        {
            nsAutoLock lock(mLock);

            entry = NS_STATIC_CAST(nsLDAPServiceEntry *,
mServers->Get(&hashKey));
            if (!entry) {
                return NS_ERROR_FAILURE;
            }
            if (!entry->PushListener(NS_STATIC_CAST(nsILDAPMessageListener *,
                                                    aListener))) {
                return NS_ERROR_FAILURE;
            }
        }

This should be done once, just before the final return NS_OK.

From IRC: you caught the stuck lock in OnLDAPMessage, so that's cool.  I'll look
for a new patch, then sr=.  Do it, almost there!

/be 

/be

Comment 45

[Brendan Eich [:brendan]]

For posterity: the lack of ref-counting of nsLDAPServiceEntry is scary.  Right
now, no one ever removes an entry from mServers, and the unlocked calls to
EstablishConnection that pass entry pointers depend on that fact.  Short of
ref-counting the table, you could maybe arrange to rename rather than remove, as
you said on IRC.  But eventually, you have to remove -- no?

(This is a question to be answered later, not an obstacle to the forthcoming 
patch going in.)

/be

Comment 46

[Leif Hedstrom]

Attachment: Patch per BE's review

Comment 47

[Brendan Eich [:brendan]]

I meant to inline the entire bodies of those Get and Set methods, moving them
from the .cpp to the .h file -- but that can happen later, and maybe we can also
avoid the local variables that work around NS_IF_ADDREF difficulties that we're
consulting scc over.

sr=[e-mail].

/be

Comment 48

[Leif Hedstrom]

Attachment: Last version of nsLDAPService.cpp

Comment 49

[Leif Hedstrom]

Attachment: Last version of nsLDAPService.h

Comment 50

[Leif Hedstrom]

Attachment: Macintoy madness, already_AddRefed no good, new nsLDAPService.cpp

Comment 51

[Leif Hedstrom]

Attachment: Yo yo yo, here's nsLDAPService.h without already_AddRefed()

Comment 52

[Brendan Eich [:brendan]]

Two leaks, both of the form:

            if (entry->GetMessage()) {
                // We already have a message, lets keep that one.
                //
                return NS_ERROR_FAILURE;
            }

That was ok when you were cheating and not addref'ing, but now....  Either add a
boolean HasMessage predicate, or use an nsCOMPtr and getter_AddRefs (sorry the
already_AddRefed idea didn't pan out -- cc'ing scc for followup).

Otherwise looks good enough to check in, so sr=[e-mail] again, once
you fix the leaks.  I did raise some questions in comments earlier today, such
as when entries should get removed, and how the code should protect them.  So
leave this bug open or (better if you transcribe enough context) file a sequel
bug and list its number here.

/be

/be

Comment 53

[Brendan Eich [:brendan]]

The other followup to-do: really inline these getters (and maybe make them XPCOM
style, if we can't have already_AddRefed; it's incongruous to see an XPCOM-style
out param passed using getter_AddRefs for operation->GetConnection, while the
entry->GetConnection uses the return value).

/be

Comment 54

[Brendan Eich [:brendan]]

Hey scc, can you read the last few patches and tell us why already_AddRefed for
return-value-style getters didn't compile on all platforms?

/be

Comment 55

[Brendan Eich [:brendan]]

Sorry, there's only one such GetMessage leak.  My mistake driving the cheesy
find dialog.

/be

Comment 56

[Leif Hedstrom]

Attachment: Final nsLDAPService.cpp...? Probably not.

Comment 57

[Leif Hedstrom]

I've created a new bug 78993 to track the inline and the issues with calling
styles in the nsLDAPServiceEntry class. I also addded BE's comment re: deletion
of entries to bug 77669.

Comment 58

[Dan Mosedale (:dmosedale, :dmose)]

Checked in.

Comment 59

[yulian chang]

reassign to Olga as QA contact

Comment 60

[olga charapaev]

nsILDAPService interface is implemented 
marking as verified