Thread synchronization problem

[Vincent Lascaux]

Hello,

I have a class that store informations and has an "empty" attribute.
Whenever I want to access this information, I first check if the class
is "empty". If so, I download the information and set the class non
empty. So I have this method:

private synchronized void assertNotEmpty()
{
if(isEmpty())
{
download();
//now isEmpty() is false
}
}

The problem is that I now require download to be multi-threaded, and
to access some of the methods of the class (it even access the
assertNotEmpty method since it is reading some informations from the
object after having started the download). This obviously result in a
deadlock.

The simple solution is to remove the synchronized, but then I may (and
this happens because "download" is rather long) download several time
the informations (and I dont want that).

How to solve this kind of problem?

Thank you

 

[Maik Heller]

Hello,

I have a class that store informations and has an "empty" attribute.
Whenever I want to access this information, I first check if the class
is "empty". If so, I download the information and set the class non
empty. So I have this method:

private synchronized void assertNotEmpty()
{
if(isEmpty())
{
download();
//now isEmpty() is false
}
}

The problem is that I now require download to be multi-threaded, and
to access some of the methods of the class (it even access the
assertNotEmpty method since it is reading some informations from the
object after having started the download). This obviously result in a
deadlock.

The simple solution is to remove the synchronized, but then I may (and
this happens because "download" is rather long) download several time
the informations (and I dont want that).

How to solve this kind of problem?

Thank you

you may not access assertNotEmpty() from download() otherwise you have
to "rearrange" your sync path.

 

[John C. Bollinger]

Hello,

I have a class that store informations and has an "empty" attribute.
Whenever I want to access this information, I first check if the class
is "empty". If so, I download the information and set the class non
empty. So I have this method:

private synchronized void assertNotEmpty()
{
if(isEmpty())
{
download();
//now isEmpty() is false
}
}

The problem is that I now require download to be multi-threaded, and
to access some of the methods of the class (it even access the
assertNotEmpty method since it is reading some informations from the
object after having started the download). This obviously result in a
deadlock.

The simple solution is to remove the synchronized, but then I may (and
this happens because "download" is rather long) download several time
the informations (and I dont want that).

That is not a solution, as you seem to recognize. It simply causes the
application to have a different set of bugs.

How to solve this kind of problem?

Redesign the class. I can't provide details, because I don't have any
idea about what your class represents or how it is currently
implemented. To be sure, you already recognize that download must not
directly or indirectly cause assertNotEmpty() to be invoked (in any
thread). It may help if you allow your code to assume some invariants
instead of programmatically checking. For instance, if download() can
only be invoked from within assertNotEmpty() as shown above, then during
the download you can assume that your instance was empty when the
download started instead of making the threads invoke assertNotEmpty().


John Bollinger
(e-mail address removed)

 

[Vincent Lascaux]

Redesign the class. I can't provide details, because I don't have any

idea about what your class represents or how it is currently
implemented. To be sure, you already recognize that download must not
directly or indirectly cause assertNotEmpty() to be invoked (in any
thread). It may help if you allow your code to assume some invariants
instead of programmatically checking. For instance, if download() can
only be invoked from within assertNotEmpty() as shown above, then during
the download you can assume that your instance was empty when the
download started instead of making the threads invoke assertNotEmpty().

Do you think it would be good design to have the following thing :

interface IData
{ void a(); void b(); void c(); }

//This class is the class where the assertNotEmpty() function was
class Data implements IData
{
//a, b, and c make no call to download
public synchronized void a() { ... }
public synchronized void b() { ... }
public synchronized void c() { ... }
public synchronized void download() { ... }
}

class DataAccessor implements IData
{
private Data inner;
private boolean empty = true;
public DataAccessor(Data inner) { this.inner = inner; }
private synchronized void assertNotEmpty() {
if(empty) {
inner.download();
empty = false;
}
}
public synchronized void a() { assertNotEmpty(); inner.a(); }
public synchronized void b() { assertNotEmpty(); inner.b(); }
public synchronized void c() { assertNotEmpty(); inner.c(); }
}

That way, download could access all the methods of the inner object without
dead locks...
What I dont like is that if I want to change my code to add a new public
function, I now have 3 classes to change instead of one... But well, that's
better than a bug

 

[John C. Bollinger]

Do you think it would be good design to have the following thing :

I cannot offer reliable design advice without knowing what requirements
the design is supposed to satisfy. You have described specific actions
that you want your code to perform. Fine, but that's not a design-level
issue. If you want design advice then I need a higher-level picture.

interface IData
{ void a(); void b(); void c(); }

//This class is the class where the assertNotEmpty() function was
class Data implements IData
{
//a, b, and c make no call to download
public synchronized void a() { ... }
public synchronized void b() { ... }
public synchronized void c() { ... }
public synchronized void download() { ... }
}

class DataAccessor implements IData
{
private Data inner;
private boolean empty = true;
public DataAccessor(Data inner) { this.inner = inner; }
private synchronized void assertNotEmpty() {
if(empty) {
inner.download();
empty = false;
}
}
public synchronized void a() { assertNotEmpty(); inner.a(); }
public synchronized void b() { assertNotEmpty(); inner.b(); }
public synchronized void c() { assertNotEmpty(); inner.c(); }
}

That way, download could access all the methods of the inner object without
dead locks...
What I dont like is that if I want to change my code to add a new public
function, I now have 3 classes to change instead of one... But well, that's
better than a bug

I think it's needlessly complex, and may not satisfy your requirements.
Earlier you wrote that Data's methods must invoke assertNotEmpty(),
which they do not do in this design. If those methods cannot perform
meaningful and appropriate work on an empty instance (or one in the
process of being downloaded) then this design cannot work. If they
_can_ perform appropriate work under those circumstances then you have
not correctly characterized your problem.

From your example above, it seems like you are trying to implement a
version of the Flyweight pattern. Perhaps you will find some useful
advice if you know the name of what you're trying to do. On the other
hand, it may be that an altogether different strategy would be better --
I just can't say because I don't know _why_ you are doing what you are
doing.


John Bollinger
(e-mail address removed)

 

[Vincent Lascaux]

I cannot offer reliable design advice without knowing what requirements

the design is supposed to satisfy. You have described specific actions
that you want your code to perform. Fine, but that's not a design-level
issue. If you want design advice then I need a higher-level picture.

Sorry about that
The Data class stores some informations that are downloaded from a computer
above the network.
I have in two data classes: one container (Datas) and the actual data (Data)
Datas acts as a map<string, Data>

Datas offers methods such as size(), getByName(string), iterator(),
add(string, Data), remove(string), removeAll()
Data offers also several methods to work (read and modify) the data
contained (I dont think it is necessary to describe the whole context).

When I ask to download a Datas object, I download only the list of names,
and dont want to download the actual content of the Data object.
When I ask to download a Data object, the information is retrieved from the
network.

These two objects are used rather intensively in the application (the only
purpose of this application is in fact to manage these datas).
In my first code, I checked whether the data was empty every time I accessed
it (like every time I did size(), I first checked and downloaded the Datas
object if it was empty). It was fine while the application was small enough,
but soon I forgot to download at some locations in the code, and maintaining
that became quite complex.
So my idea was to move this "check and download if necessary" procedure
inside the Datas and Data object. They would always be considered as not
empty for the user of the table, and when the first method is called, it
would download the data. That's where I am, with my synchronization problem.

I think it's needlessly complex, and may not satisfy your requirements.
Earlier you wrote that Data's methods must invoke assertNotEmpty(),
which they do not do in this design. If those methods cannot perform
meaningful and appropriate work on an empty instance (or one in the
process of being downloaded) then this design cannot work. If they
_can_ perform appropriate work under those circumstances then you have
not correctly characterized your problem.

The Data object cannot perform meaningful and appropriate work on empty
instance.
The idea behind this design is that if you use the Data object, then you are
responsible for downloading it if it is empty, before accessing any of the
methods.
The DataAccessor is there to ensure that for you, checking if the data has
been previously downloaded before accessing it in any way.
I agree this is complex, and I would like a more simple solution...

From your example above, it seems like you are trying to implement a
version of the Flyweight pattern. Perhaps you will find some useful
advice if you know the name of what you're trying to do. On the other
hand, it may be that an altogether different strategy would be better --
I just can't say because I don't know _why_ you are doing what you are
doing.

I'm not sure about the Flyweight pattern. Looking through google, it looks
like it is a pattern to share some common information among several objects.
That doesnt seem to be what I want to do

 

[Chris Uppal]

When I ask to download a Datas object, I download only the list of names,
and dont want to download the actual content of the Data object.
When I ask to download a Data object, the information is retrieved from
the network.

This sounds quite similar to what is sometimes called a "promise" or a "future"
(the difference is that typically they start executing in the background
straight-away, rather than waiting for the first access to start the slow
process). You may find it worthwhile to look at
java.util.concurrent.FutureTask in the SDK 1.5 beta.

If that doesn't help, then I suspect that one thing that is confusing you is
that if a thread is executing a method synchronised on some object and that
method (recursively) calls another method synchronised on the /same/ object,
then that second method is allowed to go ahead -- it doesn't block (or
deadlock). You will still need to take care that you don't start a second
download, but it should be enough to set, and check, a flag to say that the
download has started but not yet completed.

-- chris

 

[John C. Bollinger]

The Data class stores some informations that are downloaded from a computer
above the network.
I have in two data classes: one container (Datas) and the actual data (Data)
Datas acts as a map<string, Data>

Datas offers methods such as size(), getByName(string), iterator(),
add(string, Data), remove(string), removeAll()
Data offers also several methods to work (read and modify) the data
contained (I dont think it is necessary to describe the whole context).

When I ask to download a Datas object, I download only the list of names,
and dont want to download the actual content of the Data object.
When I ask to download a Data object, the information is retrieved from the
network.
OK.

[...]

So my idea was to move this "check and download if necessary" procedure
inside the Datas and Data object. They would always be considered as not
empty for the user of the table, and when the first method is called, it
would download the data. That's where I am, with my synchronization problem.
[...]

The Data object cannot perform meaningful and appropriate work on empty
instance.

In that case, your original download() method must not invoke the other
methods of Data. Such an implementation is by definition incorrect.
Once you fix that issue, you should not have a synchronization problem,
but there may still be a better design.

The idea behind this design is that if you use the Data object, then you are
responsible for downloading it if it is empty, before accessing any of the
methods.
The DataAccessor is there to ensure that for you, checking if the data has
been previously downloaded before accessing it in any way.
I agree this is complex, and I would like a more simple solution...

If your "Datas" collection can always be assumed to mediate the process
of obtaining a Data instance, then you can put the download control
there. That way you don't need to check on every method invocation, but
instead only when someone asks a Datas for a particular Data. To take
that idea all the way to its logical conclusion, you might position
Datas as a Factory class.

I'm not sure about the Flyweight pattern. Looking through google, it looks
like it is a pattern to share some common information among several objects.
That doesnt seem to be what I want to do

Well, I've read definitions of Flyweight that put their emphasis in a
different place. The key in those definitions is that a Flyweight
object is a stand-in for a heavier-weight object, so as to make it more
efficient to maintain [the equivalent of] a collection of large,
rarely-used objects. That seems a bit closer to what you're trying to
do, no?


John Bollinger
(e-mail address removed)

 

[Vincent Lascaux]

In that case, your original download() method must not invoke the other

methods of Data. Such an implementation is by definition incorrect.
Once you fix that issue, you should not have a synchronization problem,
but there may still be a better design.

My data object is a table that maintains several objects itself. I download
object by object and call a add(Object) method. This method has to be public
so that the Data object can be modified by the user of the class. It also
checks that the object added is valid (one of the rules for example is that
no two objects must have the same "name" (a property of the object)). I want
to reuse this add function (I dont want to copy and paste the body of the
function in my download method :^p )

 

[John C. Bollinger]

My data object is a table that maintains several objects itself. I download
object by object and call a add(Object) method. This method has to be public
so that the Data object can be modified by the user of the class. It also
checks that the object added is valid (one of the rules for example is that
no two objects must have the same "name" (a property of the object)). I want
to reuse this add function (I dont want to copy and paste the body of the
function in my download method :^p )

Then it IS the case that Data's methods may meaningfully be invoked on a
Data that is not fully loaded, at least sometimes. You denied that
before. In any event, a solution along the lines I suggested may still
be suitable.


John Bollinger
(e-mail address removed)