AP-24824: Addeds a refresh path in getCategories() so dev-mode extens…#70
AP-24824: Addeds a refresh path in getCategories() so dev-mode extens…#70
Conversation
…ions... ... (-Dknime.python.extension.config) re-populate categories on demand. Cache rebuild now re-derives nodes and categories from the supplier, ensuring category sets reflect dev-mode configs even if they were initialized before the config was picked up. AP-24824 (Python node do not show up in correct category in Modern UI in Dev mode)
There was a problem hiding this comment.
Pull request overview
This PR addresses AP-24824 by adding a refresh path so Python extension categories (and associated node/category caches) can be re-derived from the extension supplier on demand, improving correctness in dev-mode where custom extension config may be picked up after initial cache initialization.
Changes:
- Store the
extensionSupplierand introduce arefreshCaches()method to rebuild extensions/nodes/categories from the supplier. - Trigger cache refresh from
getCategories()when custom extension paths are present (dev-mode scenario). - Add a helper (
hasCustomExtensions()) based onPythonExtensionPreferences.
| public Collection<CategoryExtension> getCategories() { | ||
| if (hasCustomExtensions()) { | ||
| refreshCaches(); | ||
| } |
There was a problem hiding this comment.
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.
| 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(); |
There was a problem hiding this comment.
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.
| 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(); | ||
| 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| if (hasCustomExtensions()) { | ||
| refreshCaches(); | ||
| } | ||
| return m_allCategories; |
There was a problem hiding this comment.
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.
| return m_allCategories; | |
| return List.copyOf(m_allCategories); |
1 participant
…ions...
... (-Dknime.python.extension.config) re-populate categories on demand. Cache rebuild now re-derives nodes and categories from the supplier, ensuring category sets reflect dev-mode configs even if they were initialized before the config was picked up.
AP-24824 (Python node do not show up in correct category in Modern UI in Dev mode)