-
Notifications
You must be signed in to change notification settings - Fork 122
Special behaviour for temporal prefixes #1644
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
export const TEMPORAL_RESERVED_PREFIX = '__temporal_'; | ||
export const STACK_TRACE_RESERVED_PREFIX = '__stack_trace'; | ||
export const ENHANCED_STACK_TRACE_RESERVED_PREFIX = '__enhanced_stack_trace'; | ||
|
||
export const reservedPrefixes = [ | ||
TEMPORAL_RESERVED_PREFIX, | ||
STACK_TRACE_RESERVED_PREFIX, | ||
ENHANCED_STACK_TRACE_RESERVED_PREFIX, | ||
]; | ||
|
||
export function throwIfReservedName(type: string, name: string): void { | ||
const prefix = isReservedName(name); | ||
if (prefix) { | ||
throw Error(`Cannot register ${type} name: '${name}', with reserved prefix: '${prefix}'`); | ||
} | ||
} | ||
|
||
export function isReservedName(name: string): string | undefined { | ||
for (const prefix of reservedPrefixes) { | ||
if (name.startsWith(prefix)) { | ||
return prefix; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { | ||
condition, | ||
defineSignal, | ||
setDefaultQueryHandler, | ||
setDefaultSignalHandler, | ||
setHandler, | ||
} from '@temporalio/workflow'; | ||
|
||
export async function workflowWithDefaultHandlers(): Promise<void> { | ||
const complete = true; | ||
setDefaultQueryHandler(() => {}); | ||
setDefaultSignalHandler(() => {}); | ||
setHandler(defineSignal('completeSignal'), () => {}); | ||
|
||
await condition(() => complete); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,12 @@ import { | |
import { composeInterceptors } from '@temporalio/common/lib/interceptors'; | ||
import { makeProtoEnumConverters } from '@temporalio/common/lib/internal-workflow'; | ||
import type { coresdk, temporal } from '@temporalio/proto'; | ||
import { | ||
ENHANCED_STACK_TRACE_RESERVED_PREFIX, | ||
STACK_TRACE_RESERVED_PREFIX, | ||
isReservedName, | ||
throwIfReservedName, | ||
} from '@temporalio/common/lib/reserved'; | ||
import { alea, RNG } from './alea'; | ||
import { RootCancellationScope } from './cancellation-scope'; | ||
import { UpdateScope } from './update-scope'; | ||
|
@@ -249,7 +255,7 @@ export class Activator implements ActivationHandler { | |
*/ | ||
public readonly queryHandlers = new Map<string, WorkflowQueryAnnotatedType>([ | ||
[ | ||
'__stack_trace', | ||
STACK_TRACE_RESERVED_PREFIX, | ||
{ | ||
handler: () => { | ||
return this.getStackTraces() | ||
|
@@ -260,7 +266,7 @@ export class Activator implements ActivationHandler { | |
}, | ||
], | ||
[ | ||
'__enhanced_stack_trace', | ||
ENHANCED_STACK_TRACE_RESERVED_PREFIX, | ||
{ | ||
handler: (): EnhancedStackTrace => { | ||
const { sourceMap } = this; | ||
|
@@ -619,6 +625,8 @@ export class Activator implements ActivationHandler { | |
protected queryWorkflowNextHandler({ queryName, args }: QueryInput): Promise<unknown> { | ||
let fn = this.queryHandlers.get(queryName)?.handler; | ||
if (fn === undefined && this.defaultQueryHandler !== undefined) { | ||
// Do not call default query handler with reserved query name. | ||
throwIfReservedName('query', queryName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should not be throwing this way here. Code below is clearly designed to make sure that we are not throwing synchronously. Also, UI expects that we'll answer back with a list of accepted query types. Actually, I'd just skip the default handler if query name is reserved. We'll still throw below, but we'll then be going the same code path as if that wasn't a reserved type name. |
||
fn = this.defaultQueryHandler.bind(this, queryName); | ||
} | ||
// No handler or default registered, fail. | ||
|
@@ -649,17 +657,28 @@ export class Activator implements ActivationHandler { | |
throw new TypeError('Missing query activation attributes'); | ||
} | ||
|
||
const queryInput = { | ||
queryName: queryType, | ||
args: arrayFromPayloads(this.payloadConverter, activation.arguments), | ||
queryId, | ||
headers: headers ?? {}, | ||
}; | ||
|
||
// Skip interceptors if this is an internal query. | ||
if (isReservedName(queryType)) { | ||
this.queryWorkflowNextHandler(queryInput).then( | ||
(result) => this.completeQuery(queryId, result), | ||
(reason) => this.failQuery(queryId, reason) | ||
); | ||
return; | ||
} | ||
|
||
const execute = composeInterceptors( | ||
this.interceptors.inbound, | ||
'handleQuery', | ||
this.queryWorkflowNextHandler.bind(this) | ||
); | ||
execute({ | ||
queryName: queryType, | ||
args: arrayFromPayloads(this.payloadConverter, activation.arguments), | ||
queryId, | ||
headers: headers ?? {}, | ||
}).then( | ||
execute(queryInput).then( | ||
(result) => this.completeQuery(queryId, result), | ||
(reason) => this.failQuery(queryId, reason) | ||
); | ||
|
@@ -797,6 +816,8 @@ export class Activator implements ActivationHandler { | |
if (fn) { | ||
return await fn(...args); | ||
} else if (this.defaultSignalHandler) { | ||
// Do not call default signal handler with reserved signal name. | ||
throwIfReservedName('signal', signalName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely shouldn't throw here, as that means that adding reserved signals in the future would make those poison pills for any workers that don't yet know about them if there's a default signal handler registered. In fact, we should simply not enter |
||
return await this.defaultSignalHandler(signalName, ...args); | ||
} else { | ||
throw new IllegalStateError(`No registered signal handler for signal: ${signalName}`); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you fallback to unit tests for default handlers?