Merge pull request #21 from AKVorrat/timeout-setting

Handle speedtest timeouts

Author: t-muehlberger

nb-speedtest/src/SpeedTest.tsx

@@ -1,5 +1,5 @@
 import { TestResult } from './types/test-result';
-import { Component, createEffect, createSignal, For, Match, Show, Switch, on } from "solid-js";
+import { Component, createEffect, createSignal, For, Match, Show, Switch, on, createResource } from "solid-js";
 import { useTranslation } from './i18n/context';
 import { PowerBtn } from './components/PowerBtn';
 import { Stepper } from './components/Stepper';
@@ -46,7 +46,7 @@ const NBSpeedTest: Component<{ onStateChange?: (state: SpeedTestState) => void }
 
   const now = createTimeSignal(1000)
 
-  // const [metadata] = createResource(fetchMetadata)
+  const [metadata] = createResource(fetchMetadata)
 
   const restart = () => {
     setSessionID(uuidV4())
@@ -65,6 +65,10 @@ const NBSpeedTest: Component<{ onStateChange?: (state: SpeedTestState) => void }
     }))
   )
 
+  createEffect(() => {
+    console.log(`Connection Metadata: ${metadata()?.asOrganization} (ASN ${metadata()?.asn})`)
+  })
+
   const onStartClick = async () => {
     // Check privacy policy acceptance first
     if (!isPrivacyPolicyAccepted()) {

nb-speedtest/src/components/AdvancedResults/ErrorAccordion.tsx

@@ -16,7 +16,7 @@ export const ErrorAccordion: Component<{
       <div class="collapse-title text-lg font-medium peer-checked:bg-base-300 peer-checked:text-base-content">
         <div class="flex flex-row flex-wrap items-center justify-between gap-2">
           <span class="text-base lg:text-lg">{props.testResult.label}</span>
-          <div class="text-xs lg:text-sm opacity-70 flex items-center gap-2">
+          <div class="text-sm flex items-center gap-2">
             <TbExclamationCircle class="w-4 h-4 text-error" />
             <span class="text-error">{t.speedtest.error()}</span>
           </div>
@@ -34,4 +34,4 @@ export const ErrorAccordion: Component<{
       </div>
     </div>
   );
-};
+};

nb-speedtest/src/data/config.ts

@@ -7,6 +7,6 @@ const parseEnvNumber = (envVar: string | undefined, defaultValue: number): numbe
 export const config = {
 	// 59 min, 59 sec
 	repeatIntervalSec: parseEnvNumber(import.meta.env.VITE_SPEEDTEST_REPEAT_INTERVAL, 60 * 60 - 1),
-	// 270 second timeout for speedtest
-	speedtestTimeoutMs: parseEnvNumber(import.meta.env.VITE_SPEEDTEST_TIMEOUT, 3 * 60 * 1000),
+	// 5 minute timeout for speedtest
+	speedtestTimeoutMs: parseEnvNumber(import.meta.env.VITE_SPEEDTEST_TIMEOUT, 5 * 60 * 1000),
 }

nb-speedtest/src/data/test-runs.ts

@@ -29,6 +29,7 @@ export function getTestRuns(sessionID: string, routeLabeler: (letter: string, la
 			turnServerCredsApiUrl: `${d.uri}/__turn`,
 			turnServerUri: "turn.cloudflare.com:3478",
 			includeCredentials: false,
+			bandwidthAbortRequestDuration: 60 * 1000,
 			measurements: [
 				{ type: "download", bytes: 100_000, count: 2 },
 				{ type: "upload", bytes: 100_000, count: 2 },

nb-speedtest/src/util/create-speedtest.ts

@@ -5,7 +5,7 @@ import { config as appConfig } from "../data/config"
 import { SpeedtestError } from "../types/speedtest-error"
 
 type SpeedTestCallbacks = {
-	onDone?: (results: Results) => void
+	onDone?: (results: Results) => void // callbacks are called exactly once, either onDone or onError, never both
 	onError?: (error: SpeedtestError) => void
 	timeoutMs?: number
 }
@@ -38,12 +38,11 @@ export function createSpeedtest(config: ConfigOptions, callbacks: SpeedTestCallb
 
 		if (isRunning() && !isFinished()) {
 			timeoutId = setTimeout(() => {
+				if (isFinished()) return
+				setIsFinished(true)
 				const error = { message: `Speedtest timeout after ${timeoutMs}ms` }
-				console.error("Speedtest timeout", error)
 				speedTest.pause()
-				if (callbacks.onError) {
-					callbacks.onError(error)
-				}
+				callbacks.onError?.(error)
 			}, timeoutMs)
 		}
 	}
@@ -85,18 +84,29 @@ export function createSpeedtest(config: ConfigOptions, callbacks: SpeedTestCallb
 			clearTimeoutTimer()
 		}
 	}
+
 	speedTest.onFinish = () => {
 		clearTimeoutTimer()
+
+		// make sure we only ever do the callback once
+		if (isFinished()) return
 		setIsFinished(true)
-		if (callbacks.onDone) callbacks.onDone(speedTest.results)
+
+		const summary = speedTest.results.getSummary()
+		console.log("done", summary, speedTest.results.getDownloadBandwidth())
+		if (!summary.download && !summary.upload) { // we consider a speedtest with no results failed
+			callbacks.onError?.({ message: "All measurements failed" })
+		} else {
+			callbacks.onDone?.(speedTest.results)
+		}
 	}
+
+	// speedtest can return multiple errors when individual measurements fail
+	// errors are non terminal so for our purpose they are more like warnings
 	speedTest.onError = (error) => {
-		clearTimeoutTimer()
-		console.error("Speedtest error", error)
-		if (callbacks.onError) {
-			callbacks.onError({ message: error })
-		}
+		console.warn("Speedtest measurement error:", error)
 	}
+
 	onCleanup(() => {
 		clearTimeoutTimer()
 		if (speedTest?.isRunning) {

package-lock.json

@@ -15,7 +15,7 @@
   "license": "ISC",
   "description": "",
   "dependencies": {
-    "@cloudflare/speedtest": "^1.7.0",
+    "@cloudflare/speedtest": "github:cloudflare/speedtest#v1.8.0",
     "@fontsource/roboto": "^5.1.0",
     "@fontsource/source-code-pro": "^5.1.0",
     "@solid-primitives/i18n": "^2.2.1",