moquette-io / moquette

Java MQTT lightweight broker
http://moquette-io.github.io/moquette/
Apache License 2.0
2.3k stars 818 forks source link

about remove root #712

Closed BEWINDOWEB closed 1 year ago

BEWINDOWEB commented 1 year ago

confused about version: 0.16 -> CTrie -> remove method, which goes:

if (cnode.containsOnly(clientId) && topic.isEmpty() && cnode.allChildren().isEmpty()) {
      // last client to leave this node, AND there are no downstream children, remove via TNode tomb
      if (inode == this.root) {
          return inode.compareAndSet(cnode, inode.mainNode().copy()) ? Action.OK : Action.REPEAT;
      }
      TNode tnode = new TNode();
      return inode.compareAndSet(cnode, tnode) ? cleanTomb(inode, iParent) : Action.REPEAT;
 }
  1. when will inode == this.root? topic can't be empty at the beginning
  2. why if inode == this.root, then do the meaningless CAS inode.compareAndSet(cnode, inode.mainNode().copy()) ?

please help on this, thanks :)

BEWINDOWEB commented 1 year ago

@andsel 🙏🙏

andsel commented 1 year ago

Hi @BEWINDOWEB this change come into play with PR #365 and honestly I don't recall what was the exact reason for the code.

Originally the code was simpler:

if (cnode.containsOnly(clientId)) {
    TNode tnode = new TNode();
    return inode.compareAndSet(cnode, tnode) ? Action.OK : Action.REPEAT;
} else if (cnode.contains(clientId)) {
    CNode updatedCnode = cnode.copy();  

that stated as "if a cnode contains only the clientId we I'm removing then replace it with a tombstone".

BEWINDOWEB commented 1 year ago

ok, thanks anyway :)