imbajin commented on code in PR #3138:
URL: https://github.com/apache/hugegraph/pull/3138#discussion_r3759752449
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1771,10 +1800,60 @@ private void listenMetaChanges() {
this.metaManager.listenGraphClear(ConsumerWrapper.wrap(this::graphClearHandler));
}
+ /**
+ * Notify the listeners of `event` and wait for them to finish, failing if
+ * any listener did not complete successfully.
+ * <p>
+ * EventHub swallows every throwable raised by a listener and resolves the
+ * future with the number of listeners that returned normally, so waiting
+ * alone doesn't prove that the graph was registered. Comparing the
+ * notified count with the registered listener count detects the swallowed
+ * failure and lets the caller fail instead of returning a graph that is
+ * missing from the rest/gremlin server context.
+ */
private void notifyAndWaitEvent(String event, HugeGraph graph) {
- Future<?> future = this.eventHub.notify(event, graph);
+ String graphName = graph.spaceGraphName();
+ // Listeners of ANY_EVENT are notified too, so they count as expected
+ int expected = this.eventHub.listeners(event).size() +
+ this.eventHub.listeners(EventHub.ANY_EVENT).size();
+ int notified;
try {
- future.get();
+ Future<Integer> future = this.eventHub.notify(event, graph);
+ notified = future.get(EVENT_WAIT_TIMEOUT, TimeUnit.SECONDS);
Review Comment:
‼️ The timeout bounds the caller's wait, but not the event task's side
effects. `Future.get(timeout)` can throw while the `EventHub` worker remains
queued or running; the caller then closes/removes the graph, and a late
`injectGraph()` can register that closed graph and its TraversalSource
afterward. Please make cancellation and cleanup deterministic so no listener
can mutate bindings after rollback begins.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1342,19 +1353,37 @@ public HugeGraph createGraph(String graphSpace, String
name, String creator,
graph.updateTime(timeStamp);
String graphName = spaceGraphName(graphSpace, name);
+ this.graphs.put(graphName, graph);
+
+ /*
+ * Let gremlin server and rest server context add graph before the
+ * graph is published, so that a failed local binding can't leave the
+ * graph behind in meta for the other servers to converge on
+ */
+ try {
+ this.notifyAndWaitEvent(Events.GRAPH_CREATE, graph);
+ } catch (Throwable e) {
+ this.graphs.remove(graphName);
Review Comment:
‼️ This rollback removes only `GraphManager.graphs` and closes the graph.
`ContextGremlinServer.injectGraph()` updates the Gremlin graph,
TraversalSource, and global bindings in separate steps, so a later step can
throw after earlier bindings were installed; the listener-count check detects
the failure, but those partial bindings remain. Please explicitly undo the
Gremlin-side registrations (or make injection atomic) before closing the graph;
the local create failure path needs the same guarantee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
