You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Emmanuel Lecharny <el...@gmail.com> on 2011/01/06 17:26:51 UTC
Exception interceptor pb
Hi,
today, while tracking some issue in the server, I jumped into some code
in the ExceptionInterceptor that I find questionable. Let me explain
something we already discussed on IRC.
When we do a search, we go through the chain of interceptors, down to
the backend. In the backend, we process the request, and construct a
Cursor based on the search filter and the scope.
This cursor has not yet fetch any entry.
Then we go back the chain up to the SearchHandler. In the process, we
construct a list of filters to apply to each entry we will fetch. Those
filters simply select good candidates, and reject bad ones. They also
eliminate not requested attributes or values.
When we go up the chain, and process the ExceptionInterceptor, the
following code is executed :
public EntryFilteringCursor search( NextInterceptor
nextInterceptor, SearchOperationContext searchContext )
{
EntryFilteringCursor cursor = nextInterceptor.search(
searchContext );
// Check that if the cursor is empty, it's not because the DN
is invalid.
if ( !cursor.available() && !searchContext.getDn().isEmpty() &&
!subschemSubentryDn.equals( searchContext.getDn()) )
{
// We just check that the entry exists only if we didn't
found any entry
assertHasEntry( searchContext, "Attempt to search under
non-existant entry:", searchContext.getDn() );
}
return cursor;
}
The assertHasEntry method is :
private void assertHasEntry( OperationContext opContext, String
msg, DN dn ) throws LdapException
{
if ( !opContext.hasEntry( dn, ByPassConstants.HAS_ENTRY_BYPASS ) )
{
throw new LdapNoSuchObjectException( msg + dn.getName() );
}
}
And now, the problem : the cursor.available() call *always* return
false, thus we most of the case do a lookup, which is costly. Every
single search we do will always fetch N+1 entry : The N entry to return,
plus the first entry a second time.
This is useless.
The reason why this check is being done is that we need to send a
LDAP_NO_SUCH_OBJECT result if there is no entry returned.
We discussed some options we have. Currently, we have 2 :
- make the cursor return something different from false if there is an
entry, even if we don't prefetch the entry.
- postpone this test and handle the case in the SearchHandler
Option #1 implies a huge refactoring of the cursor API. Not sure we want
to do this
Option #2 is probably better. We just have to check that it's valid to
check for the absence of an entry if the candidate have been discarded
by some upper filter (for instance, if we rejected some entry because
the ACIs mandated it). In this case, should we send back a
LDAP_NO_SUCH_OBJECT?
I will experiment with Option #2, re-read the RFC, and update you.
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: Exception interceptor pb
Posted by Emmanuel Lécharny <el...@apache.org>.
On 1/6/11 7:02 PM, Alex Karasulu wrote:
> On Thu, Jan 6, 2011 at 6:26 PM, Emmanuel Lecharny<el...@gmail.com>wrote:
>
>> Hi,
>>
>> today, while tracking some issue in the server, I jumped into some code in
>> the ExceptionInterceptor that I find questionable. Let me explain something
>> we already discussed on IRC.
>>
>>
> You know I thought about this to a degree after our conversations. I think
> the Cursor is setup to be automatically positioned on the first element if
> one exists. This is why we are checking if an element is available. So
> calling available() might be the right thing to do and makes sense.
Yep. And t's the only way to go (see the mail I just sent).
Although I have no idea how to have the available() method return the
correct info.
> So even inside a block of code inside an interceptor after Cursor creation
> it might check if the underlying candidate has elements available before
> wrapping it or even adding its own filter.
This part of the code is like the dark side of the moon for me...
>
>> When we go up the chain, and process the ExceptionInterceptor, the
>> following code is executed :
>> public EntryFilteringCursor search( NextInterceptor nextInterceptor,
>> SearchOperationContext searchContext )
>> {
>> EntryFilteringCursor cursor = nextInterceptor.search( searchContext
>> );
>>
>> // Check that if the cursor is empty, it's not because the DN is
>> invalid.
>> if ( !cursor.available()&& !searchContext.getDn().isEmpty()&&
>> !subschemSubentryDn.equals( searchContext.getDn()) )
>> {
>> // We just check that the entry exists only if we didn't found
>> any entry
>> assertHasEntry( searchContext, "Attempt to search under
>> non-existant entry:", searchContext.getDn() );
>> }
>>
>> return cursor;
>> }
>>
>> The assertHasEntry method is :
>>
>> private void assertHasEntry( OperationContext opContext, String msg, DN
>> dn ) throws LdapException
>> {
>> if ( !opContext.hasEntry( dn, ByPassConstants.HAS_ENTRY_BYPASS ) )
>> {
>> throw new LdapNoSuchObjectException( msg + dn.getName() );
>> }
>> }
>>
>> And now, the problem : the cursor.available() call *always* return false,
>> thus we most of the case do a lookup, which is costly. Every single search
>> we do will always fetch N+1 entry : The N entry to return, plus the first
>> entry a second time.
>>
>>
> Hmmm why does it return the first entry a second time?
It does not return the first entry a second time. What it does is that
it read the first entry to see if it exists, and if so, the process
continues and later, while transmitting the SearchResult, we read the
first entry again. So the first entry is read twice.
>
>> This is useless.
>>
>>
> Yes it should not do this.
>
>
>> The reason why this check is being done is that we need to send a
>> LDAP_NO_SUCH_OBJECT result if there is no entry returned.
>>
>> We discussed some options we have. Currently, we have 2 :
>> - make the cursor return something different from false if there is an
>> entry, even if we don't prefetch the entry.
>> - postpone this test and handle the case in the SearchHandler
>>
>> Option #1 implies a huge refactoring of the cursor API. Not sure we want to
>> do this
>> Option #2 is probably better. We just have to check that it's valid to
>> check for the absence of an entry if the candidate have been discarded by
>> some upper filter (for instance, if we rejected some entry because the ACIs
>> mandated it). In this case, should we send back a LDAP_NO_SUCH_OBJECT?
>>
>> I will experiment with Option #2, re-read the RFC, and update you.
>>
> What worries me is that this worked just fine for years. It should not be
> failing us now: there has to be something wrong here. I can take a look at
> the code and maybe we can discuss this more together on IRC.
It works. It's just that it's not efficient. I removed the cursor.next()
and replaced it by a cursor.available(), expecting the server to avoid
fetching the first entry twice, but sadly, as available() returns false
until the entry is fetched, we still fetch the entry twice...
Btw, see my second mail for the reason why we have to handle this a the
cursor level, not on the SearchHandler level.
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: Exception interceptor pb
Posted by Alex Karasulu <ak...@apache.org>.
On Thu, Jan 6, 2011 at 6:26 PM, Emmanuel Lecharny <el...@gmail.com>wrote:
> Hi,
>
> today, while tracking some issue in the server, I jumped into some code in
> the ExceptionInterceptor that I find questionable. Let me explain something
> we already discussed on IRC.
>
>
You know I thought about this to a degree after our conversations. I think
the Cursor is setup to be automatically positioned on the first element if
one exists. This is why we are checking if an element is available. So
calling available() might be the right thing to do and makes sense.
So even inside a block of code inside an interceptor after Cursor creation
it might check if the underlying candidate has elements available before
wrapping it or even adding its own filter.
> When we do a search, we go through the chain of interceptors, down to the
> backend. In the backend, we process the request, and construct a Cursor
> based on the search filter and the scope.
>
> This cursor has not yet fetch any entry.
>
>
Cursors returned are in fact designed to prefetch. Under the hood they know
if they have anything available so they can position themselves right before
the first element they will return.
> Then we go back the chain up to the SearchHandler. In the process, we
> construct a list of filters to apply to each entry we will fetch. Those
> filters simply select good candidates, and reject bad ones. They also
> eliminate not requested attributes or values.
>
>
Exactly.
> When we go up the chain, and process the ExceptionInterceptor, the
> following code is executed :
> public EntryFilteringCursor search( NextInterceptor nextInterceptor,
> SearchOperationContext searchContext )
> {
> EntryFilteringCursor cursor = nextInterceptor.search( searchContext
> );
>
> // Check that if the cursor is empty, it's not because the DN is
> invalid.
> if ( !cursor.available() && !searchContext.getDn().isEmpty() &&
> !subschemSubentryDn.equals( searchContext.getDn()) )
> {
> // We just check that the entry exists only if we didn't found
> any entry
> assertHasEntry( searchContext, "Attempt to search under
> non-existant entry:", searchContext.getDn() );
> }
>
> return cursor;
> }
>
> The assertHasEntry method is :
>
> private void assertHasEntry( OperationContext opContext, String msg, DN
> dn ) throws LdapException
> {
> if ( !opContext.hasEntry( dn, ByPassConstants.HAS_ENTRY_BYPASS ) )
> {
> throw new LdapNoSuchObjectException( msg + dn.getName() );
> }
> }
>
> And now, the problem : the cursor.available() call *always* return false,
> thus we most of the case do a lookup, which is costly. Every single search
> we do will always fetch N+1 entry : The N entry to return, plus the first
> entry a second time.
>
>
Hmmm why does it return the first entry a second time?
> This is useless.
>
>
Yes it should not do this.
> The reason why this check is being done is that we need to send a
> LDAP_NO_SUCH_OBJECT result if there is no entry returned.
>
> We discussed some options we have. Currently, we have 2 :
> - make the cursor return something different from false if there is an
> entry, even if we don't prefetch the entry.
> - postpone this test and handle the case in the SearchHandler
>
> Option #1 implies a huge refactoring of the cursor API. Not sure we want to
> do this
> Option #2 is probably better. We just have to check that it's valid to
> check for the absence of an entry if the candidate have been discarded by
> some upper filter (for instance, if we rejected some entry because the ACIs
> mandated it). In this case, should we send back a LDAP_NO_SUCH_OBJECT?
>
> I will experiment with Option #2, re-read the RFC, and update you.
>
What worries me is that this worked just fine for years. It should not be
failing us now: there has to be something wrong here. I can take a look at
the code and maybe we can discuss this more together on IRC.
--
Alex Karasulu
My Blog :: http://www.jroller.com/akarasulu/
Apache Directory Server :: http://directory.apache.org
Apache MINA :: http://mina.apache.org
To set up a meeting with me: http://tungle.me/AlexKarasulu