📡 binder connection errors reported

jupyter-book · Nov 21, 2023 · 02fe666 · 02fe666
1 parent fb6a6da
commit 02fe666
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 125 deletions.
diff --git a/apps/demo-react/src/App.tsx b/apps/demo-react/src/App.tsx
@@ -25,7 +25,7 @@ function App() {
         repo: 'executablebooks/thebe-binder-base',
       },
     }),
-    [path],
+    [path, mode],
   );
 
   return (

diff --git a/apps/demo-react/src/ConnectionStatusTray.tsx b/apps/demo-react/src/ConnectionStatusTray.tsx
@@ -3,16 +3,15 @@ import { ThebeEventData } from 'thebe-core';
 import { useThebeServer } from 'thebe-react';
 
 export function ConnectionStatusTray() {
-  const { connecting, subscribe, unsubAll } = useThebeServer();
+  const { connecting, subscribe } = useThebeServer();
   const [status, setStatus] = useState<ThebeEventData | null>(null);
 
   useEffect(() => {
     if (!subscribe) return;
     subscribe((data: ThebeEventData) => {
       setStatus(data);
     });
-    return unsubAll;
-  }, [subscribe, unsubAll]);
+  }, [subscribe]);
 
   return (
     <div className="mono not-prose max-w-[80%] m-auto min-h-[3em] border-[1px] border-blue-500 relative pb-1">

diff --git a/apps/demo-react/src/NotebookErrorTray.tsx b/apps/demo-react/src/NotebookErrorTray.tsx
@@ -4,19 +4,17 @@ import { useThebeConfig } from 'thebe-react';
 
 export function NotebookErrorTray() {
   const { config } = useThebeConfig();
-  const [subscribed, setSubscribed] = useState<boolean>(false);
   const [errorEvent, setErrorEvent] = useState<ThebeEventData | null>(null);
 
   useEffect(() => {
-    if (!config?.events || subscribed) return;
-    config?.events?.on('error' as ThebeEventType, (event: any, data: ThebeEventData) => {
-      // report status events reelated to the server or session connection only
+    if (!config) return;
+    config?.events.on('error' as ThebeEventType, (event: any, data: ThebeEventData) => {
+      // report execution error events
       if ([EventSubject.notebook, EventSubject.cell].includes(data.subject as EventSubject)) {
         setErrorEvent(data);
       }
     });
-    setSubscribed(true);
-  }, [config, subscribed]);
+  }, [config]);
 
   if (!errorEvent) return null;
 

diff --git a/apps/demo-react/src/NotebookStatusTray.tsx b/apps/demo-react/src/NotebookStatusTray.tsx
@@ -4,19 +4,17 @@ import { useThebeConfig } from 'thebe-react';
 
 export function NotebookStatusTray() {
   const { config } = useThebeConfig();
-  const [subscribed, setSubscribed] = useState<boolean>(false);
   const [status, setStatus] = useState<ThebeEventData | null>(null);
 
   useEffect(() => {
-    if (!config?.events || subscribed) return;
+    if (!config?.events) return;
     config?.events?.on('status' as ThebeEventType, (event: any, data: ThebeEventData) => {
-      // report status events reelated to the server or session connection only
+      // report status events related to execution only
       if ([EventSubject.notebook, EventSubject.cell].includes(data.subject as EventSubject)) {
         setStatus(data);
       }
     });
-    setSubscribed(true);
-  }, [config, subscribed]);
+  }, [config]);
 
   return (
     <div className="mono not-prose max-w-[80%] m-auto min-h-[3em] border-[1px] border-blue-500 relative pb-1 mt-2">

diff --git a/packages/core/src/emitter.ts b/packages/core/src/emitter.ts
@@ -27,7 +27,7 @@ export class EventEmitter {
   }
 
   triggerError({ status, message }: { status: ErrorStatusEvent; message: string }) {
-    console.debug(`Error ${message}`);
+    console.debug(`Error [${this._subject}][${this._id}] ${message}`);
 
     this._config.events.trigger(ThebeEventType.error, {
       subject: this._subject,

diff --git a/packages/core/src/server.ts b/packages/core/src/server.ts
@@ -32,6 +32,14 @@ async function responseToJson(res: Response) {
   return json as RestAPIContentsResponse;
 }
 
+function ensureString(errorLike: any): string {
+  if (typeof errorLike === 'string') return errorLike;
+  if (errorLike.message) return errorLike.message;
+  if (errorLike.status && errorLike.statusText)
+    return `${errorLike.status} - ${errorLike.statusText}`;
+  return JSON.stringify(errorLike);
+}
+
 class ThebeServer implements ServerRuntime, ServerRestAPI {
   readonly id: string;
   readonly config: Config;
@@ -185,11 +193,12 @@ class ThebeServer implements ServerRuntime, ServerRestAPI {
       });
       // eslint-disable-next-line no-empty
     } catch (err: any) {
+      const message = `Server not reachable (${serverSettings.baseUrl}) - ${err}`;
       this.events.triggerError({
         status: ErrorStatusEvent.error,
-        message: `Server not reachable (${serverSettings.baseUrl}) - ${err}`,
+        message,
       });
-      this.rejectReadyFn?.(err);
+      this.rejectReadyFn?.(message);
       return;
     }
 
@@ -219,7 +228,7 @@ class ThebeServer implements ServerRuntime, ServerRestAPI {
         });
         this.resolveReadyFn?.(this);
       },
-      (err) => this.rejectReadyFn?.(err),
+      (err) => this.rejectReadyFn?.(ensureString(err)),
     );
   }
 
@@ -265,7 +274,7 @@ class ThebeServer implements ServerRuntime, ServerRestAPI {
         });
         this.resolveReadyFn?.(this);
       },
-      (err) => this.rejectReadyFn?.(err),
+      (err) => this.rejectReadyFn?.(ensureString(err)),
     );
   }
 
@@ -358,114 +367,125 @@ class ThebeServer implements ServerRuntime, ServerRestAPI {
             });
             this.resolveReadyFn?.(this);
           },
-          (err) => this.rejectReadyFn?.(err),
+          (err) => this.rejectReadyFn?.(ensureString(err)),
         );
         // else drop out of this block and request a new session
       }
     }
 
     // TODO we can get rid of one level of promise here?
-    const requestPromise: Promise<void> = new Promise((resolveRequest, rejectRequest) => {
-      // Talk to the binder server
-      const state: { status: StatusEvent } = {
-        status: ServerStatusEvent.launching,
-      };
-      const es = new EventSource(urls.build);
-      this.events.triggerStatus({
-        status: state.status,
-        message: `Opened connection to binder: ${urls.build}`,
-      });
+    // const requestPromise: Promise<void> = new Promise((resolveRequest, rejectRequest) => {
+    // Talk to the binder server
+    const state: { status: StatusEvent } = {
+      status: ServerStatusEvent.launching,
+    };
+    const es = new EventSource(urls.build);
+    this.events.triggerStatus({
+      status: state.status,
+      message: `Opened connection to binder: ${urls.build}`,
+    });
 
-      // handle errors
-      es.onerror = (evt: Event) => {
-        console.error(`Lost connection to binder: ${urls.build}`, evt);
-        es?.close();
-        state.status = ErrorStatusEvent.error;
-        const data = (evt as MessageEvent)?.data;
-        const phase = data ? data.phase : 'unknown';
-        const message = data ? data.message : 'no message';
-        this.events.triggerError({
-          status: ErrorStatusEvent.error,
-          message: `phase: ${phase} - ${message} - Lost connection to binder`,
-        });
-        rejectRequest(evt);
-      };
-
-      es.onmessage = async (evt: MessageEvent<string>) => {
-        const msg: {
-          // TODO must be in Jupyterlab types somewhere
-          phase: string;
-          message: string;
-          url: string;
-          token: string;
-        } = JSON.parse(evt.data);
-
-        const phase = msg.phase?.toLowerCase() ?? '';
-        switch (phase) {
-          case 'failed':
+    // handle errors
+    es.onerror = (evt: Event) => {
+      console.error(`Lost connection to binder: ${urls.build}`, evt);
+      es?.close();
+      state.status = ErrorStatusEvent.error;
+      const data = (evt as MessageEvent)?.data;
+      const phase = data ? data.phase : 'unknown';
+      const message = `Lost connection to binder: ${urls.build}\nphase: ${phase} - ${
+        data ? data.message : 'no message'
+      }`;
+      this.events.triggerError({
+        status: ErrorStatusEvent.error,
+        message,
+      });
+      // rejectRequest(message);
+      this.rejectReadyFn?.(message);
+    };
+
+    es.onmessage = async (evt: MessageEvent<string>) => {
+      const msg: {
+        // TODO must be in Jupyterlab types somewhere
+        phase: string;
+        message: string;
+        url: string;
+        token: string;
+      } = JSON.parse(evt.data);
+
+      const phase = msg.phase?.toLowerCase() ?? '';
+      switch (phase) {
+        case 'failed':
+          es?.close();
+          state.status = ErrorStatusEvent.error;
+          this.events.triggerError({
+            status: ErrorStatusEvent.error,
+            message: `Binder: failed to build - ${urls.build} - ${msg.message}`,
+          });
+          console.log('reject', msg);
+          // rejectRequest(msg.message);
+          this.rejectReadyFn?.(msg.message);
+          break;
+        case 'ready':
+          {
             es?.close();
-            state.status = ErrorStatusEvent.error;
-            this.events.triggerError({
-              status: ErrorStatusEvent.error,
-              message: `Binder: failed to build - ${urls.build} - ${msg.message}`,
+
+            const settings: ServerSettings = {
+              baseUrl: msg.url,
+              wsUrl: 'ws' + msg.url.slice(4),
+              token: msg.token,
+              appendToken: true,
+            };
+
+            const serverSettings = ServerConnection.makeSettings(settings);
+            const kernelManager = new KernelManager({ serverSettings });
+            this.sessionManager = new SessionManager({
+              kernelManager,
+              serverSettings,
             });
-            rejectRequest(msg);
-            break;
-          case 'ready':
-            {
-              es?.close();
-
-              const settings: ServerSettings = {
-                baseUrl: msg.url,
-                wsUrl: 'ws' + msg.url.slice(4),
-                token: msg.token,
-                appendToken: true,
-              };
-
-              const serverSettings = ServerConnection.makeSettings(settings);
-              const kernelManager = new KernelManager({ serverSettings });
-              this.sessionManager = new SessionManager({
-                kernelManager,
-                serverSettings,
-              });
-
-              if (this.config.savedSessions.enabled) {
-                saveServerInfo(this.config.savedSessions, urls.build, this.id, serverSettings);
-                console.debug(
-                  `thebe:server:connectToServerViaBinder Saved session for ${this.id} at ${urls.build}`,
-                );
-              }
-
-              // promise has already been returned to the caller
-              // so we can await here
-              await this.sessionManager.ready;
-
-              this.userServerUrl = `${msg.url}?token=${msg.token}`;
-
-              state.status = ServerStatusEvent.ready;
-              this.events.triggerStatus({
-                status: state.status,
-                message: `Binder server is ready: ${msg.message}`,
-              });
-
-              resolveRequest();
+
+            if (this.config.savedSessions.enabled) {
+              saveServerInfo(this.config.savedSessions, urls.build, this.id, serverSettings);
+              console.debug(
+                `thebe:server:connectToServerViaBinder Saved session for ${this.id} at ${urls.build}`,
+              );
             }
-            break;
-          default:
+
+            // promise has already been returned to the caller
+            // so we can await here
+            await this.sessionManager.ready;
+
+            this.userServerUrl = `${msg.url}?token=${msg.token}`;
+
+            state.status = ServerStatusEvent.ready;
             this.events.triggerStatus({
               status: state.status,
-              message: `Binder is: ${phase} - ${msg.message}`,
+              message: `Binder server is ready: ${msg.message}`,
             });
-        }
-      };
-    });
 
-    return requestPromise.then(
-      () => {
-        this.resolveReadyFn?.(this);
-      },
-      (err) => this.rejectReadyFn?.(err),
-    );
+            // resolveRequest();
+            this.resolveReadyFn?.(this);
+          }
+          break;
+        default:
+          this.events.triggerStatus({
+            status: state.status,
+            message: `Binder is: ${phase} - ${msg.message}`,
+          });
+      }
+    };
+    // });
+
+    // return this.ready;
+
+    // return requestPromise.then(
+    //   () => {
+    //     this.resolveReadyFn?.(this);
+    //   },
+    //   (reason: any) => {
+    //     console.log('top level reject');
+    //     this.rejectReadyFn?.(ensureString(reason));
+    //   },
+    // );
   }
 
   //

diff --git a/packages/react/src/ThebeServerProvider.tsx b/packages/react/src/ThebeServerProvider.tsx
@@ -2,6 +2,7 @@ import React, { useCallback, useContext, useEffect, useMemo, useState } from 're
 import type {
   Config,
   CoreOptions,
+  EventSubject,
   RepoProviderSpec,
   ThebeEventCb,
   ThebeEventData,
@@ -61,27 +62,26 @@ export function ThebeServerProvider({
     [core, options],
   );
 
-  // register an error handler immedately on the config changing, this is done as a
-  // side effect so tht we can un-register on unmount
+  // create an iniital server
   useEffect(() => {
-    if (!core || !thebeConfig) return;
+    if (!core || !thebeConfig || server) return;
+    const svr = new core.ThebeServer(thebeConfig);
+
+    // register an error handler immedately
     const handler = (evt: string, data: ThebeEventData) => {
       const subjects = [
         core.EventSubject.server,
         core.EventSubject.session,
         core.EventSubject.kernel,
       ];
-      if (data.subject && subjects.includes(data.subject) && data.id === server?.id)
+      if (data.subject && subjects.includes(data.subject)) {
         setError(`${data.status} - ${data.message}`);
+      }
     };
+    // TODO we need a way to unsubscribe from this that does not cause
+    // error events to be missed due to rerenders
     thebeConfig.events.on(core.ThebeEventType.error, handler);
-    return () => thebeConfig.events.off(core.ThebeEventType.error, handler);
-  }, [core, thebeConfig]);
-
-  // create an iniital server
-  useEffect(() => {
-    if (!core || !thebeConfig || server) return;
-    setServer(new core.ThebeServer(thebeConfig));
+    setServer(svr);
   }, [core, thebeConfig, server]);
 
   // Once the core is loaded, connect to a server
-Original file line number
+Diff line change
@@ Expand Up / @@ -25,7 +25,7 @@ function App() { @@
             repo: 'executablebooks/thebe-binder-base',
           },
         }),
-        [path],
+        [path, mode],
       );
       return (
@@ Expand Down @@