Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle plugin errors #453

Merged
merged 6 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/core/src/Source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {
IVolumeImmutable,
MutableData,
Plugin,
PluginErrors,
PluginModuleDefinition,
Serialiser,
SerialiserModuleDefinition,
Expand Down Expand Up @@ -36,6 +37,7 @@ export default class Source {
#ignorePages: string[];
#workflows: SourceWorkflow[] = [];
#schedule: SourceSchedule;
#pluginErrors: any = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update the type of this when we have identified what we want to do with the errors. They are logged to the console but it would be nice to provide an API that can be used to see the errors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth leaving this till after we merged in Next 13 changes... as right now, it's easy to rebase that branch, when we confine other changes to core and not change any of the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine...just pushed a change that will allow you to see the plugin errors when using the list sources admin API. Should be good enough for us to diagnose problems.


config: MutableData<Record<string, unknown>>;
serialiser: Serialiser;
Expand Down Expand Up @@ -188,12 +190,15 @@ export default class Source {

async start() {
this.serialiser = await bindSerialiser(this.#serialisers);
this.#pluginApi = await bindPluginMethods(this.#plugins);
this.#pluginApi = await bindPluginMethods(this.#plugins, this.trackPluginErrors.bind(this));
this.#worker = this.#createWorker();
this.#worker.on(EVENT.UPDATE, async ({ data: { pages, symlinks, data } }) => {
this.config = createConfig(data);
this.#emitter.emit(EVENT.UPDATE, { pages, symlinks, data });
});
this.#worker.on(EVENT.TRACK, async ({ data: { errors, lifecycleMethod } }) => {
this.#pluginErrors[`${lifecycleMethod}`] = errors;
});
}

async isOwner(filePath: string) {
Expand Down Expand Up @@ -230,7 +235,12 @@ export default class Source {
}

async restart() {
console.log(this.#pluginErrors);
DavieReid marked this conversation as resolved.
Show resolved Hide resolved
this.stop();
await this.start();
}

trackPluginErrors(errors: PluginErrors, lifecycleMethod: string) {
this.#pluginErrors[`${lifecycleMethod}`] = errors;
}
}
7 changes: 5 additions & 2 deletions packages/core/src/WorkerSubscription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ export enum EVENT {
ERROR = 'ERROR',
EXIT = 'EXIT',
START = 'START',
UPDATE = 'UPDATE'
UPDATE = 'UPDATE',
TRACK = 'TRACK'
}

const textDecoder = new TextDecoder('utf8');
Expand Down Expand Up @@ -52,9 +53,11 @@ export default class WorkerSubscription {
return () => this.#emitter.off(type, handler);
}

#onNext = ({ type, data }: { data: Uint8Array; type: 'init' | 'message' }) => {
#onNext = ({ type, data }: { data: Uint8Array; type: 'init' | 'message' | 'track' }) => {
if (type === 'message') {
this.#emitter.emit(EVENT.UPDATE, { data: JSON.parse(textDecoder.decode(data)) });
} else if (type === 'track') {
this.#emitter.emit(EVENT.TRACK, { data: JSON.parse(textDecoder.decode(data)) });
} else if (type === 'init') {
this.#emitter.emit(EVENT.START);
if (data) {
Expand Down
17 changes: 14 additions & 3 deletions packages/core/src/plugin/createPluginAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import type {
Page,
Plugin,
PluginModuleDefinition,
Serialiser
Serialiser,
TrackPluginErrorCallback
} from '@jpmorganchase/mosaic-types';
import PluginError from '@jpmorganchase/mosaic-plugins/PluginError';

import loadDefinitionModules from './loadDefinitionModules.js';
import pluginRunner from './pluginRunner.js';
Expand Down Expand Up @@ -43,7 +45,8 @@ function createProxyBaseAPI<ConfigData>(): Plugin<Page, ConfigData> {
}

export default async function createPluginAPI<PluginInput, ConfigData = Record<string, unknown>>(
plugins: PluginModuleDefinition[]
plugins: PluginModuleDefinition[],
track?: TrackPluginErrorCallback
): Promise<Plugin<Page, ConfigData>> {
const loadedPlugins: LoadedPlugin[] | LoadedSerialiser[] = await loadDefinitionModules(plugins);
const baseObj = createProxyBaseAPI<ConfigData>();
Expand All @@ -64,6 +67,9 @@ export default async function createPluginAPI<PluginInput, ConfigData = Record<s
...args
);
} catch (e) {
if (e instanceof PluginError) {
throw e;
}
throw new Error(e);
}
return result;
Expand All @@ -73,14 +79,19 @@ export default async function createPluginAPI<PluginInput, ConfigData = Record<s
return async (...args: [PluginInput, Record<string, unknown>]) => {
let result;
try {
result = await pluginRunner(
const { result: pluginResult, errors } = await pluginRunner(
{
loadedPlugins: loadedPlugins as LoadedPlugin[],
lifecycleName: String(propOrLifecycleName)
},
...args
);
track?.(errors, String(propOrLifecycleName));
result = pluginResult;
} catch (e) {
if (e instanceof PluginError) {
throw e;
}
throw new Error(e);
}
return result;
Expand Down
52 changes: 11 additions & 41 deletions packages/core/src/plugin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import type {
Page,
Plugin,
PluginModuleDefinition,
Serialiser
Serialiser,
TrackPluginErrorCallback
} from '@jpmorganchase/mosaic-types';

import loadPlugins from './createPluginAPI.js';
Expand Down Expand Up @@ -35,58 +36,27 @@ export async function bindSerialiser(serialisers): Promise<Serialiser> {
};
}

export async function bindPluginMethods(plugins: PluginModuleDefinition[]): Promise<Plugin> {
const pluginApi = await loadPlugins<Page[] | TDataOut | IVolumeMutable>(plugins);
export async function bindPluginMethods(
plugins: PluginModuleDefinition[],
track?: TrackPluginErrorCallback
): Promise<Plugin> {
const pluginApi = await loadPlugins<Page[] | TDataOut | IVolumeMutable>(plugins, track);

return {
async $afterSource(pages, args) {
let result;
try {
result = await pluginApi.$afterSource(pages, args);
} catch (e) {
throw new Error(e);
}
const result = await pluginApi.$afterSource(pages, args);
return result;
},
async shouldClearCache(lastAfterUpdateReturn, args) {
let result;
try {
result = await pluginApi.shouldClearCache(lastAfterUpdateReturn, args);
} catch (e) {
throw new Error(e);
}
const result = await pluginApi.shouldClearCache(lastAfterUpdateReturn, args);
return result;
},
async afterUpdate(mutableFilesystem, args) {
let result;
try {
result = await pluginApi.afterUpdate(mutableFilesystem, args);
} catch (e) {
throw new Error(e);
}
const result = await pluginApi.afterUpdate(mutableFilesystem, args);
return result;
},
async $beforeSend(mutableFilesystem, args) {
let result;
try {
result = await pluginApi.$beforeSend(mutableFilesystem, args);
} catch (e) {
throw new Error(e);
}
return result;
},
async saveContent(
filePath: string,
data: unknown,
sourceOptions: Record<string, unknown>,
args
) {
let result;
try {
result = await pluginApi.saveContent(filePath, data, sourceOptions, args);
} catch (e) {
throw new Error(e);
}
const result = await pluginApi.$beforeSend(mutableFilesystem, args);
return result;
}
};
Expand Down
62 changes: 31 additions & 31 deletions packages/core/src/plugin/pluginRunner.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import type { LoadedPlugin } from '@jpmorganchase/mosaic-types';

import PluginError from '../PluginError.js';
import path from 'node:path';
import type { LoadedPlugin, PluginErrors } from '@jpmorganchase/mosaic-types';
import PluginError from '@jpmorganchase/mosaic-plugins/PluginError';

export default async function pluginRunner(
{ loadedPlugins, lifecycleName }: { loadedPlugins: LoadedPlugin[]; lifecycleName: string },
input,
...args
) {
let transformedInput = input;
const pluginErrors: PluginErrors = [];

for (const plugin of loadedPlugins) {
try {
Expand All @@ -20,48 +21,47 @@ export default async function pluginRunner(
continue;
}

// console.debug(
// `[Mosaic] Applying plugin method \`${lifecycleName}\`${lifecycleName.startsWith('$') ? ' (in a child worker)' : ''} for '${plugin.modulePath}'.`
// );

// eslint-disable-next-line no-await-in-loop
const result = await plugin[lifecycleName](
lifecycleName === '$afterSource' ? transformedInput : input,
...args,
plugin.options
);

if (
result &&
lifecycleName !== '$afterSource' &&
lifecycleName !== 'shouldClearCache' &&
lifecycleName !== 'saveContent'
) {
if (result && lifecycleName !== '$afterSource' && lifecycleName !== 'shouldClearCache') {
console.warn(
`[Mosaic] \`${lifecycleName}\` plugin should not return a value - this lifecycle phase expects mutation to occur directly on the filesystem instance. This will be ignored.`
);
}

if (lifecycleName === 'saveContent' && !result) {
console.warn(
`[Mosaic] \`${lifecycleName}\` plugin returned a falsy value - this result has been discarded.`
);
// eslint-disable-next-line no-continue
continue;
}

transformedInput = result;
} catch (e) {
// This check will stop nested errors from ending up with multiple 'Plugin X threw an exception' headers from
// being prefixed to the messages
if (e instanceof PluginError) {
throw e;
}
throw new PluginError(
`Plugin '${plugin.modulePath}' threw an exception running \`${lifecycleName}\`. See below:
${e.stack}`
} catch (exception) {
const pluginName = path.posix.basename(
plugin.modulePath,
path.posix.extname(plugin.modulePath)
);
console.log(
DavieReid marked this conversation as resolved.
Show resolved Hide resolved
`[Mosaic][Plugin] '${pluginName}' threw an exception running \`${lifecycleName}\``
);
console.log(`[Mosaic][Plugin] stack: ${exception.stack}`);

if (exception instanceof PluginError) {
exception.lifecycleMethod = lifecycleName;
exception.pluginModulePath = plugin.modulePath;
exception.pluginName = pluginName;
pluginErrors.push(exception);
} else {
// create a new plugin error
const pluginError = new PluginError(exception.message);
pluginError.pluginModulePath = plugin.modulePath;
pluginError.lifecycleMethod = lifecycleName;
pluginError.pluginName = pluginName;
pluginErrors.push(pluginError);
}

// process this lifecycle for the rest of the loaded plugins
continue;
}
}
return transformedInput;
return { result: transformedInput, errors: pluginErrors };
}
20 changes: 10 additions & 10 deletions packages/core/src/plugin/serialiserRunner.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { LoadedSerialiser } from '@jpmorganchase/mosaic-types';
import path from 'node:path';

import PluginError from '../PluginError.js';
import PluginError from '@jpmorganchase/mosaic-plugins/PluginError';

const isPathWithExtension = (filePath: string) => {
return typeof filePath === 'string' && path.extname(filePath).length > 1;
Expand Down Expand Up @@ -31,17 +30,18 @@ export default async function serialiserRunner(
const result = await serialiser[serialiserMethod](fullPath, ...args, serialiser.options);
return result;
} catch (e) {
// This check will stop nested errors from ending up with multiple 'Plugin X threw an exception' headers from
// being prefixed to the messages
if (e instanceof PluginError) {
e.lifecycleMethod = serialiserMethod;
e.pluginModulePath = serialiser.modulePath;
throw e;
}
throw new PluginError(
`Serialiser '${serialiser.modulePath}' threw an exception calling \`serialiser.${serialiserMethod}\` on '${fullPath}'.

Is this the correct serialiser to use for this file type? See error below:
${e.stack}`
);

// create a new plugin error
const pluginError = new PluginError(e.message);
pluginError.stack = e.stack;
pluginError.pluginModulePath = serialiser.modulePath;
pluginError.lifecycleMethod = serialiserMethod;
throw pluginError;
}
}
throw new Error(`Could not find a suitable serialiser for file '${fullPath}'.`);
Expand Down
12 changes: 10 additions & 2 deletions packages/core/src/worker/Source.worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,22 @@ import fs from 'fs';
import { DirectoryJSON, Volume } from 'memfs';
import { switchMap, tap } from 'rxjs';

import type { Page, WorkerData } from '@jpmorganchase/mosaic-types';
import type { Page, PluginErrors, WorkerData } from '@jpmorganchase/mosaic-types';

import FileAccess from '../filesystems/FileAccess.js';
import MutableVolume from '../filesystems/MutableVolume.js';
import createConfig from '../helpers/createConfig.js';
import { bindPluginMethods, bindSerialiser } from '../plugin/index.js';
import createSourceObservable from './helpers/createSourceObservable.js';

const trackPluginErrors = (errors: PluginErrors, lifecycleMethod: string) => {
const errorsBuffer = Buffer.from(JSON.stringify({ errors, lifecycleMethod }));
parentPort.postMessage({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plugin methods that start with a $ run inside a child process and must communicate with the parent thread by posting a message.

type: 'track',
data: errorsBuffer
});
};

const workerData: WorkerData<{ cache: boolean }> = unTypedWorkerData;

if (isMainThread) {
Expand All @@ -22,7 +30,7 @@ if (isMainThread) {
let config;
let filesystem;
const serialiser = await bindSerialiser(workerData.serialisers);
const pluginApi = await bindPluginMethods(workerData.plugins);
const pluginApi = await bindPluginMethods(workerData.plugins, trackPluginErrors);
const cachePath = path.join(process.cwd(), '.tmp', '.cache', `${workerData.name}.json`);

if (workerData.options.cache !== false) {
Expand Down
30 changes: 30 additions & 0 deletions packages/types/src/Plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,36 @@ import type { MutableData, ImmutableData } from './MutableData.js';
export type LoadedPlugin = Partial<Plugin<Page>> & PluginModuleDefinition;
export type LifecycleMethod = keyof Plugin<Page>;

export type PluginErrorDescriptor = {
/**
* The plugin lifecycle method running when the error occurred
*/
lifecycleMethod: string;
/**
* The path to the loaded plugin module
*/
pluginModulePath: string;
/**
* The name of the plugin
*/
pluginName: string;
/**
* The name of the error
*/
name: string;
/**
* The path of the file that triggered the plugin error
*/
fullPath?: string;
};

export type PluginErrors = Array<PluginErrorDescriptor>;

export type TrackPluginErrorCallback = (
errors: Array<PluginErrorDescriptor>,
lifecycleMethod: string
) => void;

/**
* Plugins are lifecycle-based hooks that are called on every source at different stages.
* Consumers will never need to invoke a lifecycle method; but for technical clarity - when a lifecycle method is called,
Expand Down