[JENKINS-57273] Jenkins/Nodes.addNode could replace an existing node, but always fired onCreated #4004
Conversation
| @@ -210,6 +215,7 @@ public Boolean call() throws Exception { | |||
| if (exists) { | |||
| // TODO there is a theoretical race whereby the node instance is updated/removed after lock release | |||
| persistNode(node); | |||
| // TODO should this fireOnUpdated? | |||
There was a problem hiding this comment.
It looks like the only use of this method in core is via Node.save, and even in other cases, since old and new would be the same object, it seems a bit weird to call fireOnUpdated. That said, the only NodeListener I could find that even uses NodeListener.onUpdated is in audit-log, so it's hard to say how the event is used in practice or if the change would be problematic.
There was a problem hiding this comment.
OK. Feel free to suggest an update to the comment with your research.
| @@ -131,10 +131,11 @@ public void addNode(final @Nonnull Node node) throws IOException { | |||
| if (node != oldNode) { | |||
| // TODO we should not need to lock the queue for adding nodes but until we have a way to update the | |||
| // computer list for just the new node | |||
| AtomicReference<Node> old = new AtomicReference<>(); | |||
There was a problem hiding this comment.
AIUI you are introducing this variable because oldNode is obtained without a lock and so might not be the actual node that we end up replacing. If so, then I think on line 152 oldNode should be replaced with old.get() for the same reason (although that could still be incorrect as mentioned on line 143).
There was a problem hiding this comment.
AIUI you are introducing this variable because
oldNodeis obtained without a lock and so might not be the actual node that we end up replacing.
Not as such, I just needed a writable cell to stash the result of put, and the Queue.withLock overrides take closures rather than the AutoCloseable trick so I could not just use a blank final variable.
on line 152
oldNodeshould be replaced withold.get()for the same reason
Perhaps. That would be a different change though. This PR is merely fixing the notification.
oleg-nenashev
added
the
needs-jira-issue
jglick
changed the title
jglick
removed
the
needs-jira-issue
jglick
added
the
ready-for-merge
|
Will merge tomorrow if there is no negative feedback |
JENKINS-57273
Buglet I noticed while working on something else.
Proposed changelog entries
NodeListener.onCreatedwas called whenJenkins.addNodeorNodes.addNodeactually replaced an existing node.