jpos / jPOS

jPOS Project
http://jpos.org
GNU Affero General Public License v3.0
599 stars 458 forks source link

fix: ChannelAdaptor disconnect lock now a new Object. Fixes #585 #586

Open agustiza opened 3 months ago

barspi commented 3 months ago

Doig so e git archeology, it used to be a generic private Object but was changed into TRUE https://github.com/jpos/jPOS/commit/301e36a603203df8113e5eba01de97607cd456b7

The explanation in the commits mentions that the Object would complicate using a persistent space such as JESpace (not much explanation as to how.. serialization of a ChannelAdaptor? and the comment itself shows some doubts about the usefulness)

I agree that the boolean is troublesome… maybe a string object would be the best of both worlds?

agustiza commented 3 months ago

Good find!

Still I'm not sure the intention was to share the lock over a space (and it wouldn't really make sense if serialized over the wire).

Seems like the intention was to simply use a boolean for the space signaling and the lock was included under a overly broad replace.

alcarraz commented 3 months ago

It seems that the disconnectLock variable, is only used in a synchronized block, wouldn't be better to fix this with a reentrant lock?

agustiza commented 3 months ago

Sure, they are pretty much equivalent in this case with the added benefit of being Loom proof should the sender or receiver become green threads in the future.

    protected void disconnect () {
        // do not synchronize on this as both Sender and Receiver can deadlock against a thread calling stop()
        disconnectLock.lock();
        try {
            SpaceUtil.wipe(sp, ready);
            channel.disconnect();
        } catch (Exception e) {
            getLog().warn("disconnect", e);
        } finally {
            disconnectLock.unlock();
        }
    }