Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agustiza
Copy link

No description provided.

@barspi
Copy link
Contributor

barspi commented Mar 12, 2024

Doig so e git archeology, it used to be a generic private Object but was changed into TRUE
301e36a

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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();
        }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants