deadlock in TestCrash

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

deadlock in TestCrash

Michael McCandless-2
RAMFile has this method:

  final synchronized byte[] addBuffer(int size) {
    byte[] buffer = newBuffer(size);
    if (directory!=null)
      synchronized (directory) {             // Ensure addition of
buffer and adjustment to directory size are atomic wrt directory
        buffers.add(buffer);
        directory.sizeInBytes += size;
        sizeInBytes += size;
      }
    else
      buffers.add(buffer);
    return buffer;
  }

But in working on LUCENE-2095, I'm seeing a deadlock, because that
method first syncs on RAMFile and then tries to sync on directory,
while the MockRAMDirectory.crash method, and I think maybe other
places, do the reverse.  I remember also hitting a different deadlock
from this in the past, and working around it.

I'd like to break the deadlock by changing RAMFile to this:

  final byte[] addBuffer(int size) {
    byte[] buffer = newBuffer(size);
    synchronized(this) {
      buffers.add(buffer);
      sizeInBytes += size;
    }

    if (directory != null) {
      synchronized (directory) {
        directory.sizeInBytes += size;
      }
    }
    return buffer;
  }

But, then, the addition of the buffer and the change of sizeInBytes is
no longer atomic wrt the directory (as the comment says).

So my question is... is this change OK?  Why is/was it so crucial that
the sizeInBytes & RAMFile's buffers really change only atomically?
Ie, it seems harmless if RAMDirectory.sizeInBytes() might not include
a buffer that's in the process of being added...?

(I do see TestRAMDirectory.testRAMDirectorySize requires this
atomicity, but, I can fix the test to only check the size in the
end...).

Mike

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: deadlock in TestCrash

Jason Rutherglen
If syncing on directory is not important, then sizeInBytes can be an AtomicLong?

On Fri, Nov 27, 2009 at 8:37 AM, Michael McCandless
<[hidden email]> wrote:

> RAMFile has this method:
>
>  final synchronized byte[] addBuffer(int size) {
>    byte[] buffer = newBuffer(size);
>    if (directory!=null)
>      synchronized (directory) {             // Ensure addition of
> buffer and adjustment to directory size are atomic wrt directory
>        buffers.add(buffer);
>        directory.sizeInBytes += size;
>        sizeInBytes += size;
>      }
>    else
>      buffers.add(buffer);
>    return buffer;
>  }
>
> But in working on LUCENE-2095, I'm seeing a deadlock, because that
> method first syncs on RAMFile and then tries to sync on directory,
> while the MockRAMDirectory.crash method, and I think maybe other
> places, do the reverse.  I remember also hitting a different deadlock
> from this in the past, and working around it.
>
> I'd like to break the deadlock by changing RAMFile to this:
>
>  final byte[] addBuffer(int size) {
>    byte[] buffer = newBuffer(size);
>    synchronized(this) {
>      buffers.add(buffer);
>      sizeInBytes += size;
>    }
>
>    if (directory != null) {
>      synchronized (directory) {
>        directory.sizeInBytes += size;
>      }
>    }
>    return buffer;
>  }
>
> But, then, the addition of the buffer and the change of sizeInBytes is
> no longer atomic wrt the directory (as the comment says).
>
> So my question is... is this change OK?  Why is/was it so crucial that
> the sizeInBytes & RAMFile's buffers really change only atomically?
> Ie, it seems harmless if RAMDirectory.sizeInBytes() might not include
> a buffer that's in the process of being added...?
>
> (I do see TestRAMDirectory.testRAMDirectorySize requires this
> atomicity, but, I can fix the test to only check the size in the
> end...).
>
> Mike
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: deadlock in TestCrash

Michael McCandless-2
True... I'll make that change too.

Mike

On Fri, Nov 27, 2009 at 12:56 PM, Jason Rutherglen
<[hidden email]> wrote:

> If syncing on directory is not important, then sizeInBytes can be an AtomicLong?
>
> On Fri, Nov 27, 2009 at 8:37 AM, Michael McCandless
> <[hidden email]> wrote:
>> RAMFile has this method:
>>
>>  final synchronized byte[] addBuffer(int size) {
>>    byte[] buffer = newBuffer(size);
>>    if (directory!=null)
>>      synchronized (directory) {             // Ensure addition of
>> buffer and adjustment to directory size are atomic wrt directory
>>        buffers.add(buffer);
>>        directory.sizeInBytes += size;
>>        sizeInBytes += size;
>>      }
>>    else
>>      buffers.add(buffer);
>>    return buffer;
>>  }
>>
>> But in working on LUCENE-2095, I'm seeing a deadlock, because that
>> method first syncs on RAMFile and then tries to sync on directory,
>> while the MockRAMDirectory.crash method, and I think maybe other
>> places, do the reverse.  I remember also hitting a different deadlock
>> from this in the past, and working around it.
>>
>> I'd like to break the deadlock by changing RAMFile to this:
>>
>>  final byte[] addBuffer(int size) {
>>    byte[] buffer = newBuffer(size);
>>    synchronized(this) {
>>      buffers.add(buffer);
>>      sizeInBytes += size;
>>    }
>>
>>    if (directory != null) {
>>      synchronized (directory) {
>>        directory.sizeInBytes += size;
>>      }
>>    }
>>    return buffer;
>>  }
>>
>> But, then, the addition of the buffer and the change of sizeInBytes is
>> no longer atomic wrt the directory (as the comment says).
>>
>> So my question is... is this change OK?  Why is/was it so crucial that
>> the sizeInBytes & RAMFile's buffers really change only atomically?
>> Ie, it seems harmless if RAMDirectory.sizeInBytes() might not include
>> a buffer that's in the process of being added...?
>>
>> (I do see TestRAMDirectory.testRAMDirectorySize requires this
>> atomicity, but, I can fix the test to only check the size in the
>> end...).
>>
>> Mike
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]