-
Notifications
You must be signed in to change notification settings - Fork 45
fix: add StandaloneAopContextHook to pre-create ContextProto advice #400
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
Open
jerryliang64
wants to merge
2
commits into
eggjs:master
Choose a base branch
from
jerryliang64:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import type { EggContext, EggContextLifecycleContext, LoadUnitInstance } from '@eggjs/tegg-runtime'; | ||
| import type { EggProtoImplClass, LifecycleHook } from '@eggjs/tegg'; | ||
| import { PrototypeUtil, ObjectInitType } from '@eggjs/tegg'; | ||
| import { AspectInfoUtil } from '@eggjs/tegg/aop'; | ||
| import { EggPrototype, TeggError } from '@eggjs/tegg-metadata'; | ||
|
|
||
| export interface EggPrototypeWithClazz extends EggPrototype { | ||
| clazz?: EggProtoImplClass; | ||
| } | ||
|
|
||
| export interface ProtoToCreate { | ||
| name: string; | ||
| proto: EggPrototype; | ||
| } | ||
|
|
||
| /** | ||
| * AopContextHook for standalone mode. | ||
| * Pre-creates ContextProto advice objects when a new context is initialized. | ||
| * This ensures that advice objects are available when AOP methods are called. | ||
| */ | ||
| export class StandaloneAopContextHook implements LifecycleHook<EggContextLifecycleContext, EggContext> { | ||
|
Contributor
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. 這個放在 aop runtime 裏是不是更加合適? |
||
| private requestProtoList: Array<ProtoToCreate> = []; | ||
|
|
||
| constructor(loadUnitInstances: LoadUnitInstance[]) { | ||
| for (const loadUnitInstance of loadUnitInstances) { | ||
| const iterator = loadUnitInstance.loadUnit.iterateEggPrototype(); | ||
| for (const proto of iterator) { | ||
| const protoWithClazz = proto as EggPrototypeWithClazz; | ||
| const clazz = protoWithClazz.clazz; | ||
| if (!clazz) continue; | ||
| const aspects = AspectInfoUtil.getAspectList(clazz); | ||
| for (const aspect of aspects) { | ||
| for (const advice of aspect.adviceList) { | ||
| const adviceProto = PrototypeUtil.getClazzProto(advice.clazz) as EggPrototype | undefined; | ||
| if (!adviceProto) { | ||
| throw TeggError.create(`Aop Advice(${advice.clazz.name}) not found in loadUnits`, 'advice_not_found'); | ||
| } | ||
| if (adviceProto.initType === ObjectInitType.CONTEXT) { | ||
| this.requestProtoList.push({ | ||
| name: advice.name, | ||
| proto: adviceProto, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async preCreate(_: EggContextLifecycleContext, ctx: EggContext): Promise<void> { | ||
| for (const proto of this.requestProtoList) { | ||
| ctx.addProtoToCreate(proto.name, proto.proto); | ||
| } | ||
| } | ||
| } | ||
87 changes: 87 additions & 0 deletions
87
standalone/standalone/test/fixtures/singleton-aop-module/Tool.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| import { SingletonProto, Inject } from '@eggjs/tegg'; | ||
| import { | ||
| Advice, | ||
| AdviceContext, | ||
| IAdvice, | ||
| Pointcut, | ||
| } from '@eggjs/tegg/aop'; | ||
|
|
||
| /** | ||
| * This is a ContextProto Advice (default for @Advice decorator) | ||
| * This simulates the user's CachedToolCallAdvice scenario | ||
| */ | ||
| @Advice() | ||
| export class ToolCallAdvice implements IAdvice<Tool> { | ||
| private callCount = 0; | ||
|
|
||
| async beforeCall(ctx: AdviceContext<Tool>): Promise<void> { | ||
| this.callCount++; | ||
| ctx.args[0] = `[advised:${this.callCount}] ${ctx.args[0]}`; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * This is a SingletonProto that uses a ContextProto Advice | ||
| * This is the exact scenario that causes "EggObject not found" error | ||
| */ | ||
| @SingletonProto() | ||
| export class Tool { | ||
| @Pointcut(ToolCallAdvice) | ||
| async execute(input: string): Promise<string> { | ||
| return `Tool executed: ${input}`; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A SingletonProto that uses another SingletonProto Tool | ||
| * IMPORTANT: Both are Singletons, so in subsequent requests: | ||
| * - QueryNode is not re-created | ||
| * - Tool is not re-fetched via getOrCreateEggObject | ||
| * - ContextInitiator.init(Tool) is NOT called | ||
| * - Tool's ContextProto Advice is NOT created | ||
| * - AOP fails with "EggObject not found" | ||
| */ | ||
| @SingletonProto() | ||
| export class QueryNode { | ||
| @Inject() | ||
| tool: Tool; | ||
|
|
||
| async run(input: string): Promise<string> { | ||
| // This calls tool.execute() which has AOP | ||
| return await this.tool.execute(input); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A singleton that stores bound functions (simulates langchain graph) | ||
| * This is the key scenario that causes the bug: | ||
| * 1. Graph is created in request A, stores bound function nodeObj.execute.bind(nodeObj) | ||
| * 2. Request A ends, ContextProto Advice is destroyed | ||
| * 3. Request B uses the Graph, calls the stored bound function | ||
| * 4. The bound function's `this` (nodeObj) has a Tool that uses ContextProto Advice | ||
| * 5. AOP tries to access Advice, but it doesn't exist in request B's context | ||
| */ | ||
| @SingletonProto() | ||
| export class Graph { | ||
| private boundExecute: ((input: string) => Promise<string>) | null = null; | ||
| private _initialized = false; | ||
|
|
||
| get initialized(): boolean { | ||
| return this._initialized; | ||
| } | ||
|
|
||
| // Simulate langchain's addNode - stores bound function ONLY ONCE | ||
| setBoundExecute(fn: (input: string) => Promise<string>) { | ||
| if (!this._initialized) { | ||
| this.boundExecute = fn; | ||
| this._initialized = true; | ||
| } | ||
| } | ||
|
|
||
| async execute(input: string): Promise<string> { | ||
| if (!this.boundExecute) { | ||
| throw new Error('boundExecute not set'); | ||
| } | ||
| return await this.boundExecute(input); | ||
| } | ||
| } |
32 changes: 32 additions & 0 deletions
32
standalone/standalone/test/fixtures/singleton-aop-module/main.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { ContextProto, Inject } from '@eggjs/tegg'; | ||
| import { Runner, MainRunner } from '@eggjs/tegg/standalone'; | ||
| import { Graph, QueryNode } from './Tool'; | ||
| import { EggContainerFactory } from '@eggjs/tegg-runtime'; | ||
|
|
||
| @Runner() | ||
| @ContextProto() | ||
| export class Main implements MainRunner<string> { | ||
| @Inject() | ||
| graph: Graph; | ||
|
|
||
| async main(): Promise<string> { | ||
| // First request: set up the graph with bound execute function | ||
| // Key: We get QueryNode ONLY on first request, not on subsequent requests | ||
| if (!this.graph.initialized) { | ||
| // Get QueryNode only during graph initialization (first request) | ||
| const queryNodeObj = await EggContainerFactory.getOrCreateEggObjectFromClazz(QueryNode); | ||
| const queryNode = queryNodeObj.obj as QueryNode; | ||
| this.graph.setBoundExecute(queryNode.run.bind(queryNode)); | ||
| } | ||
|
|
||
| // Execute through the graph | ||
| // On subsequent requests: | ||
| // - Graph is reused (Singleton) | ||
| // - boundExecute is already set, points to QueryNode from first request | ||
| // - QueryNode.run() calls Tool.execute() which has AOP | ||
| // - AOP needs ToolCallAdvice (ContextProto) | ||
| // - Without the fix: ToolCallAdvice doesn't exist in current context -> ERROR | ||
| // - With the fix: StandaloneAopContextHook pre-creates ToolCallAdvice -> SUCCESS | ||
| return await this.graph.execute('test-input'); | ||
| } | ||
| } |
6 changes: 6 additions & 0 deletions
6
standalone/standalone/test/fixtures/singleton-aop-module/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "name": "singleton-aop-module", | ||
| "eggModule": { | ||
| "name": "singletonAopModule" | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The interfaces
EggPrototypeWithClazzandProtoToCreateappear to be internal implementation details forStandaloneAopContextHook. Sinceindex.tsusesexport * from './src/StandaloneAopContextHook', these interfaces are exposed as part of the package's public API. To maintain a clean public API and signal that these are for internal use, it's better to define them without exporting.