fix: correct polling issues [DET-5095] (#2036)

(cherry picked from commit 1a018f8)
determined-ai · Mar 4, 2021 · af5c6b8 · af5c6b8
1 parent 37b0b75
commit af5c6b8
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 68 deletions.
diff --git a/webui/react/src/hooks/usePolling.ts b/webui/react/src/hooks/usePolling.ts
@@ -3,41 +3,55 @@ import { useCallback, useEffect, useRef } from 'react';
 const DEFAULT_DELAY = 5000;
 
 type PollingFn = (() => Promise<void>) | (() => void);
+type StopFn = () => void;
 
 interface PollingOptions {
   delay?: number;
 }
 
-const usePolling = (pollingFn: PollingFn, { delay }: PollingOptions = {}): (() => void) => {
-  const func = useRef<PollingFn>(pollingFn);
+const usePolling = (pollingFn: PollingFn, { delay }: PollingOptions = {}): StopFn => {
+  const savedPollingFn = useRef<PollingFn>(pollingFn);
   const timer = useRef<NodeJS.Timeout>();
-  const active = useRef(true);
-
-  const stopPolling = useCallback(() => {
-    active.current = false;
+  const active = useRef(false);
 
+  const clearTimer = useCallback(() => {
     if (timer.current) {
       clearTimeout(timer.current);
       timer.current = undefined;
     }
   }, []);
 
-  const runPolling = useCallback(async (): Promise<void> => {
-    await func.current();
+  const poll = useCallback(async (): Promise<void> => {
+    await savedPollingFn.current();
 
     if (active.current) {
-      timer.current = setTimeout(() => runPolling(), delay || DEFAULT_DELAY);
+      timer.current = setTimeout(() => {
+        timer.current = undefined;
+        poll();
+      }, delay || DEFAULT_DELAY);
     }
-  }, [ delay, func ]);
+  }, [ delay ]);
+
+  const startPolling = useCallback(() => {
+    clearTimer();
+    active.current = true;
+    poll();
+  }, [ clearTimer, poll ]);
+
+  const stopPolling = useCallback(() => {
+    active.current = false;
+    clearTimer();
+  }, [ clearTimer ]);
 
-  // Update polling function if a new one is passed through.
+  // Update polling function if a new one is passed in.
   useEffect(() => {
-    if (func.current !== pollingFn) func.current = pollingFn;
-  }, [ pollingFn ]);
+    savedPollingFn.current = pollingFn;
+    if (!active.current) startPolling();
+  }, [ pollingFn, startPolling ]);
 
-  // Start polling upon mounting.
+  // Start polling when mounted and stop polling when umounted.
   useEffect(() => {
-    runPolling();
+    startPolling();
     return () => stopPolling();
     /* eslint-disable-next-line react-hooks/exhaustive-deps */
   }, []);

diff --git a/webui/react/src/pages/TrialDetails.tsx b/webui/react/src/pages/TrialDetails.tsx
@@ -157,7 +157,6 @@ const TrialDetailsComp: React.FC = () => {
   }, [ upgradedConfig ]);
 
   const columns = useMemo(() => {
-
     const checkpointRenderer = (_: string, record: Step) => {
       if (record.checkpoint && hasCheckpointStep(record)) {
         const checkpoint = {
@@ -225,6 +224,53 @@ const TrialDetailsComp: React.FC = () => {
       });
   }, [ showFilter, trial?.workloads ]);
 
+  const fetchExperimentDetails = useCallback(async () => {
+    if (!experimentId) return;
+
+    try {
+      const response = await getExperimentDetails(
+        { id: experimentId },
+        { signal: canceler.signal },
+      );
+      setExperiment(response);
+
+      // Experiment id does not exist in route, reroute to the one with it
+      if (!experimentIdParam) {
+        history.replace(paths.trialDetails(trialId, experimentId));
+      }
+
+      // Default to selecting config search metric only.
+      const searcherName = response.config?.searcher?.metric;
+      const defaultMetric = metricNames.find(metricName => {
+        return metricName.name === searcherName && metricName.type === MetricType.Validation;
+      });
+      const defaultMetrics = defaultMetric ? [ defaultMetric ] : [];
+      setDefaultMetrics(defaultMetrics);
+      const initMetrics = storage.getWithDefault(storageTableMetricsKey || '', defaultMetrics);
+      setDefaultMetrics(defaultMetrics);
+      setMetrics(initMetrics);
+    } catch (e) {
+      if (axios.isCancel(e)) return;
+      handleError({
+        error: e,
+        message: 'Failed to load experiment details.',
+        publicMessage: 'Failed to load experiment details.',
+        publicSubject: 'Unable to fetch Trial Experiment Detail',
+        silent: false,
+        type: ErrorType.Api,
+      });
+    }
+  }, [
+    canceler,
+    experimentId,
+    experimentIdParam,
+    history,
+    metricNames,
+    storage,
+    storageTableMetricsKey,
+    trialId,
+  ]);
+
   const fetchTrialDetails = useCallback(async () => {
     try {
       const response = await getTrialDetails({ id: trialId }, { signal: canceler.signal });
@@ -366,6 +412,10 @@ If the problem persists please contact support.',
 
   const stopPolling = usePolling(fetchTrialDetails);
 
+  useEffect(() => {
+    fetchExperimentDetails();
+  }, [ fetchExperimentDetails ]);
+
   useEffect(() => {
     if (trialDetails.data && terminalRunStates.has(trialDetails.data.state)) {
       stopPolling();
@@ -392,58 +442,6 @@ If the problem persists please contact support.',
     }
   }, [ setFreshContinueConfig ]);
 
-  useEffect(() => {
-    if (experimentId === undefined) return;
-
-    const fetchExperimentDetails = async () => {
-      try {
-        const response = await getExperimentDetails(
-          { id: experimentId },
-          { signal: canceler.signal },
-        );
-        setExperiment(response);
-
-        // Experiment id does not exist in route, reroute to the one with it
-        if (!experimentIdParam) {
-          history.replace(paths.trialDetails(trialId, experimentId));
-        }
-
-        // Default to selecting config search metric only.
-        const searcherName = response.config?.searcher?.metric;
-        const defaultMetric = metricNames.find(metricName => {
-          return metricName.name === searcherName && metricName.type === MetricType.Validation;
-        });
-        const defaultMetrics = defaultMetric ? [ defaultMetric ] : [];
-        setDefaultMetrics(defaultMetrics);
-        const initMetrics = storage.getWithDefault(storageTableMetricsKey || '', defaultMetrics);
-        setDefaultMetrics(defaultMetrics);
-        setMetrics(initMetrics);
-      } catch (e) {
-        if (axios.isCancel(e)) return;
-        handleError({
-          error: e,
-          message: 'Failed to load experiment details.',
-          publicMessage: 'Failed to load experiment details.',
-          publicSubject: 'Unable to fetch Trial Experiment Detail',
-          silent: false,
-          type: ErrorType.Api,
-        });
-      }
-    };
-
-    fetchExperimentDetails();
-  }, [
-    canceler,
-    experimentId,
-    experimentIdParam,
-    history,
-    metricNames,
-    trialDetails.source,
-    trialId,
-    storage,
-    storageTableMetricsKey,
-  ]);
-
   if (isNaN(trialId)) return <Message title={`Invalid Trial ID ${trialIdParam}`} />;
   if (trialDetails.error !== undefined) {
     const message = isNotFound(trialDetails.error) ?