Is this code Thread safe.(Changing the refernce of an instance variable )
Hi, I am confused whether the following piece of code is thread safe.
I am writing a utility in which i use a BufferedWriter and it is a class variable(static).
Now this class variable is changed by other thread while some threads may be using it to write data.
Class OurWriter
{
private static BufferedWriter ourWriter = new BufferedWriter(.....................);
public static write(String data)
{
ourWriter.write(data);
}
public static synchronized createNewWriter()
{
ourWriter = new BufferedWriter(.....................);
/*
Also old writer is flushed and closed.
What my query "Is it possible that some thread might have cached
the reference to the old BufferedWriter and may try to use earlier reference
(like the way primitives are cached by multiple threads) ? "
*/
}
}
I dunt know whether this question makes much sense, but m very confused about this. Thanking in Adavance.
Regards,
Suddu
> What my query "Is it possible that some thread might
> t have cached
> the reference to the old BufferedWriter and may try
> y to use earlier reference
> (like the way primitives are cached by multiple
> e threads) ? "
Yes, it's possible.
Threads can have their own local copies of varialbes. Variables declared volatile must always be read from and writtten to the shared copy (not a local copy). When thread enters or exits a sync block, it must sync up its local copies and the shared copy.
Thread T1 could call write and get a local copy of ourWriter. Then T2 could call createNewWriter. It will have to update the shared copy of ourWriter when it leaveds that synced method.
However, since write is not synced, T1 doesn't need to read from the shared copy. It can keep using its local copy of the reference, which points to the old Writer object.
That's not your only problem though.
Because write is not synced, it can be executing concurrently with createNewWriter. I'm not sure if that can necessarily cause any additional problems that aren't already present due to local/shared mismatch, but it's probably not what you want.
I agree with vanilla_lorax. The write method must be synchronized. The current code is open for race conditions.
This can happen.
1) createNewWriter() is called
2) createNewWriter closed the old writer
3) write(String data) is called
4) write tries to write, and throws an exception
5) createNewWriter creates a new writer
Kaj
Thanks vanilla_lorax/kajbj for the prompt response.
That clears my doubt. I agree that I need to synchronize and I will synchronize.
But say I don't close the writer in the createNewWriter() method.
Now Can making ourWriter volatile in the following piece of code would help me in the threads getting appropriate reference?
private static BufferedWriter ourWriter = new BufferedWriter(.....................);
Cheers,
Suddu
> But say I don't close the writer in the
> createNewWriter() method.
Since you don't say where you do close the writer, or whether it synced, etc., I can't comment on the safety of that.
> Now Can making ourWriter volatile in the following
> piece of code would help me in the threads getting
> appropriate reference?
You don't make it volatile in a particular bit of code. It's either volatile or it's not. It's a property of the variable that applies to the variable at all times.
And no, it wouldn't help you. In createNewWriter, it's possible for the ourWriter = new ... line to put the reference to the new writer into the variable before the writer's constructor has completed. You must synchronize all access to the writer.
hi sudarshan,
greetings !
as per my knowledge :)if your writer is shared then the WRITE method must be synchronized else you will get garbled o/p if two threads try to write the data. isnt it ?
second point is you can write a getWriter method to return the latest writer. call this method within the write(String data) method before writing the data (which being synch), you will always ensure that the data is always written using latest writer. that is no possibility for T1 to call getWriter and preempted and then T2 to call createNewWriter. before T1 writes.
and as per my understanding always close the writer otherwise the stream may get corrupted (it seems that createNewWriter is where you will close the old writer)
let me know if im wrong
cheers
amey
Ameyas thanks for the update.
But BufferedWriter it self synchronizes and acquires the lock before writing, so i dunt think that the output shud get garbed.
Your second point "second point is you can write a getWriter method to return the latest writer " wasn't very clear to me. If you synch the write(String data) method then before enetring into this method itself the thread needs to read frm the main memory wherein it would come in synch with the latest writer (the changed writer)
Please let me know if i didn't understood u properly.
cheers,
Suddu
> But BufferedWriter it self synchronizes and acquires
> the lock before writing, so i dunt think that the
> output shud get garbed.
hmmm didnt know that ..Thanks!
> Your second point "second point is you can write a
> getWriter method to return the latest writer "
> wasn't very clear to me. If you synch the
> e write(String data) method then before enetring
> into this method itself the thread needs to read frm
> the main memory wherein it would come in synch with
> the latest writer (the changed writer)
on more thinking i take that off. ... imagine T1 calls the write method
1) you call getWriter and store the reference in some variable
2) T1 gets preempted
3) T2 calls createNewWriter() and the writer is now changed
4) T1 gets in control again but has the reference to the old writer which might have been closed by the createNewWriter() call
but then i'm now confused the same problem will be faced by the current code.
1) T1 calls write method and it will go into blocked state.
2) T2 can call createNewWriter()
The writer is changed while the I/O may not be complete yet.
can anyone clarify what will happen in such case ?
cheers
amey
