Not at all; it was a good thing since my idea was to show people how to
do it right, not kinda-sorta right. :-)
Also now people can see just how much boilerplate code can be removed
with the new approach you're taking.
Bela Ban wrote:
> Didn't mean to be a nitpicker... :-)
>
> Brian Stansberry wrote:
>> Bela Ban wrote:
>>>
>>> Brian Stansberry wrote:
>>>> That should be fine. There's been a race between the thread that calls
>>>> get state and the JGroups thread that sets the state. An app that
>>>> properly handles state transfer needs to handle that race by doing
>>>> something like this:
>>>>
>>>> if (ch.getState()) {
>>>> synchronized(state_tfer_lock) {
>>>> if (!state_set) {
>>>> state_tfer_lock.wait();
>>>> }
>>>> }
>>>> }
>>> Hopefully nobody has written code that does this:
>>>
>>> boolean got_state=ch.getState();
>>> if(got_state) {
>>> synchronized(state_tfer_lock) {
>>> state_tfer_lock.wait();
>>> }
>>> }
>>>
>>> , forgetting about the state_set.
>>>
>>>
>>> Note that your code above is not really kosher either, as the
>>> recommended practice is to loop on the state_set flag, e.g.
>>>
>>> while(!state_set)
>>> state_tfer_lock.wait();
>>>
>>
>> Busted! Thanks, I should have looked at our real code (which does it
>> that way) rather than being lazy and relying on memory. :( Particularly
>> since the reason I wrote the code detail was to serve as a mini-tutorial
>> for interested readers.
>>
>> Here's a real impl from the JBoss AS ClusterPartition class:
>>
>> long start, stop;
>> isStateSet = false;
>> start = System.currentTimeMillis();
>> boolean rc = channel.getState(null, getStateTransferTimeout());
>> if (rc)
>> {
>> synchronized (channelLock)
>> {
>> while (!isStateSet)
>> {
>> if (setStateException != null)
>> throw setStateException;
>>
>> try
>> {
>> channelLock.wait();
>> }
>> catch (InterruptedException iex) {}
>> }
>> }
>> stop = System.currentTimeMillis();
>> log.info("serviceState was retrieved successfully (in " + (stop -
>> start) + " milliseconds)");
>> }
>>
>> Note the business with "setStateException". If the setState(...) method
>> catches an exception it sets that field so the main thread is aware of
>> the problem. Here we throw it and fail the deployment of the
>> ClusterPartition.
>>
>
--
Brian Stansberry
Lead, AS Clustering
JBoss, a division of Red Hat
brian.stansberry@...
|