Skip to content

AP-24824: Addeds a refresh path in getCategories() so dev-mode extens… #70

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.

Sign up for GitHub

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

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

AP-24824: Addeds a refresh path in getCategories() so dev-mode extens… #70

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions ...ython3.nodes/src/main/java/org/knime/python3/nodes/extension/ExtensionNodeSetFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
import org.knime.core.webui.node.view.NodeView;
import org.knime.core.webui.node.view.NodeViewFactory;
import org.knime.python3.nodes.DelegatingNodeModel;
import org.knime.python3.nodes.PythonExtensionPreferences;
import org.knime.python3.nodes.dialog.DelegatingJsonSettingsDataService;
import org.knime.python3.nodes.dialog.JsonFormsNodeDialog;
import org.knime.python3.nodes.ports.PythonPortTypeRegistry;
Expand All @@ -108,6 +109,8 @@ public abstract class ExtensionNodeSetFactory implements NodeSetFactory, Categor

private static final Map<String, KnimeExtension> ALL_EXTENSIONS = new ConcurrentHashMap<>();

private final Supplier<Stream<? extends KnimeExtension>> m_extensionSupplier;

private final Map<String, KnimeExtension> m_extensions;

private final Map<NodeId, ExtensionNode> m_allNodes;
Expand All @@ -120,17 +123,31 @@ public abstract class ExtensionNodeSetFactory implements NodeSetFactory, Categor
* @param extensionSupplier supplier for a {@link Stream} of {@link KnimeExtension} objects.
*/
protected ExtensionNodeSetFactory(final Supplier<Stream<? extends KnimeExtension>> extensionSupplier) {
m_extensions = extensionSupplier.get().collect(toMap(KnimeExtension::getId, Function.identity()));
ALL_EXTENSIONS.putAll(m_extensions);
m_extensionSupplier = extensionSupplier;
m_extensions = new HashMap<>();
m_allNodes = new HashMap<>();
m_allCategories = new ArrayList<>();
for (var extension : m_extensions.values()) {
refreshCaches();
}

private synchronized void refreshCaches() {
var extensions = m_extensionSupplier.get().collect(toMap(KnimeExtension::getId, Function.identity()));
m_extensions.clear();
m_extensions.putAll(extensions);
ALL_EXTENSIONS.putAll(extensions);
m_allNodes.clear();
Comment on lines +134 to +138
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshCaches() updates ALL_EXTENSIONS via putAll() but never removes entries that are no longer provided by the supplier. If custom extension paths change during dev-mode, ALL_EXTENSIONS can retain stale KnimeExtension instances indefinitely (memory leak) and DynamicExtensionNodeFactory may still resolve removed extensions. Consider clearing ALL_EXTENSIONS first (or retaining only current keys) before repopulating.

Copilot uses AI. Check for mistakes.
m_allCategories.clear();
for (var extension : extensions.values()) {
var extensionId = extension.getId();
extension.getNodes().forEach(n -> m_allNodes.put(new NodeId(extensionId, n.getId()), n));
extension.getCategories().forEach(c -> m_allCategories.add(c));
}
}
Comment on lines +133 to 145
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshCaches() mutates HashMap/ArrayList instances (clear()+put/add) while other public methods (e.g., getNodeFactoryIds/getCategoryPath) read them without synchronization. Since getCategories() can now trigger refreshCaches(), concurrent access can lead to data races or ConcurrentModificationException. Consider rebuilding into new immutable maps/lists and atomically swapping references (e.g., volatile fields), or synchronizing readers on the same lock.

Copilot uses AI. Check for mistakes.

private static boolean hasCustomExtensions() {
return PythonExtensionPreferences.getPathsToCustomExtensions().findAny().isPresent();
}

@Override
public final Collection<String> getNodeFactoryIds() {
return m_allNodes.entrySet().stream().filter(entry -> !entry.getValue().isHidden())
Expand Down Expand Up @@ -164,6 +181,9 @@ public final ConfigRO getAdditionalSettings(final String id) {

@Override
public Collection<CategoryExtension> getCategories() {
if (hasCustomExtensions()) {
refreshCaches();
}
Comment on lines 183 to +186
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In dev-mode, hasCustomExtensions() will stay true, so getCategories() will call refreshCaches() on every invocation. Since refreshCaches() re-parses extensions (see PurePythonNodeSetFactory.parseExtension -> PythonExtensionParser.parseExtension), this can add significant I/O/CPU overhead when the UI asks for categories repeatedly. Consider refreshing only when the custom extension path set has changed (cache a fingerprint/last-modified) or exposing an explicit invalidate mechanism.

Copilot uses AI. Check for mistakes.
return m_allCategories;
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCategories() returns the internal mutable m_allCategories list, and refreshCaches() clears/repopulates that same list instance. Any caller that keeps a reference or iterates while a refresh happens can observe unexpected mutations or hit ConcurrentModificationException. Prefer returning an unmodifiable snapshot (e.g., List.copyOf(...)) and avoid mutating a list that has been handed out.

Suggested change
return m_allCategories;
return List.copyOf(m_allCategories);

Copilot uses AI. Check for mistakes.
}

Expand Down
Loading