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

Async Derive leaks request context to other requests if aot:true #466

Closed
Mkassabov opened this issue Feb 6, 2024 · 10 comments
Closed

Async Derive leaks request context to other requests if aot:true #466

Mkassabov opened this issue Feb 6, 2024 · 10 comments
Labels
bug Something isn't working pending Should be resolved in next release

Comments

@Mkassabov
Copy link
Contributor

What version of Elysia.JS is running?

0.8.16

What platform is your computer?

Microsoft Windows NT 10.0.22621.0 x64

What steps can reproduce the bug?

have an async derive hook in an elysia instance with aot: false;

What is the expected behavior?

async derive in aot:false environment should not leak contexts between requests

What do you see instead?

context from newer requests leaks into the handlers of old requests

Additional information

reproduction code:

import { Elysia } from "elysia";

const app = new Elysia({ aot: false })
	.derive(() => new Promise((resolve) => setTimeout(resolve, 1000)))
	.get("/test", ({ request }) => {
		console.log(request.url);
	});

app.handle(new Request("http://localhost:1025/test"));
app.handle(new Request("http://localhost:1025/bad"));

the above code, when run, logs http://localhost:1025/bad, expected behavior is to log http://localhost:1025/test;

@Mkassabov Mkassabov added the bug Something isn't working label Feb 6, 2024
@Mkassabov
Copy link
Contributor Author

worth noting this also extends to request validation, etc. It seems the context of the new request just entirely replaces the context of the old request.

@SaltyAom
Copy link
Member

Fixed on #465, waiting for merge.

@MatthewAry
Copy link
Contributor

When using derive .derive(async () => {}) as in this contrived example, I get a TS error Type 'Promise<void>' is not assignable to type 'Record<string, unknown>. It seems there is a problem with signature of the return type for derive which is breaking the MaybePromise part of the return type.

@Mkassabov
Copy link
Contributor Author

When using derive .derive(async () => {}) as in this contrived example, I get a TS error Type 'Promise<void>' is not assignable to type 'Record<string, unknown>. It seems there is a problem with signature of the return type for derive which is breaking the MaybePromise part of the return type.

I originally ran into this when it was an async function that returned a Record<string, unknown>. I just tried to find the most minimal repro possible to demonstrate its a problem. it probably would've been better to have the set timeout use () => resolve({}) instead of just resolve

@MatthewAry
Copy link
Contributor

Circling back to this, the issue I ran into is present on 1.0.0-beta.2 I don't think it's fixed on #465

@Mkassabov
Copy link
Contributor Author

@MatthewAry

Circling back to this, the issue I ran into is present on 1.0.0-beta.2 I don't think it's fixed on #465

do you have a repro for this? the reproduction above works fine in 1.0.0-beta.2

@MatthewAry
Copy link
Contributor

I currently do not have a sharable reproduction, but I have definitely tracked down the issue.

So the built version of Elysia 1.0.0-beta.2 has a file elysia/dist/dynamic-handle-UZxzFa2z.d.ts which gives derive a definition of

    derive<const Derivative extends Record<string, unknown>>(transform: (context: Prettify<Context<Metadata['schema'], Singleton>>) => MaybePromise<Derivative> extends {
        store: any;
    } ? never : Derivative): Elysia<BasePath, Scoped, {
        decorator: Singleton['decorator'];
        store: Singleton['store'];
        derive: Prettify<Singleton['derive'] & Awaited<Derivative>>;
        resolve: Singleton['resolve'];
    }, Definitions, Metadata>;

It's being set here

elysia/src/index.ts

Lines 4497 to 4519 in ff5d5ac

derive<const Derivative extends Record<string, unknown>>(
transform: (
context: Prettify<Context<Metadata['schema'], Singleton>>
) => MaybePromise<Derivative> extends { store: any }
? never
: Derivative
): Elysia<
BasePath,
Scoped,
{
decorator: Singleton['decorator']
store: Singleton['store']
derive: Prettify<Singleton['derive'] & Awaited<Derivative>>
resolve: Singleton['resolve']
},
Definitions,
Metadata
> {
// @ts-ignore
transform.$elysia = 'derive'
return this.onTransform(transform as any) as any
}

and I've determined that my error is coming from the ternary in the callback's return type for derive which is MaybePromise<Derivative> extends { store: any } ? never : Derivative.

Note that the Derivative type is defined as Record<string, unknown> and because the TS Ternary condition can't evaluate to true with something as simple as derive(async () => {}), I can get my error.

I made a PR to fix the issue #484 which gets rid of the ternary, I'm not really sure what it's purpose was to begin with but I could be missing something since I'm not as familiar with the codebase as @SaltyAom or other contributors.

@MatthewAry
Copy link
Contributor

I've updated my PR. #488

@SaltyAom
Copy link
Member

Merged #488.

If resolved, please leave the issue opened until release on next stable release.

@SaltyAom SaltyAom added the pending Should be resolved in next release label Feb 26, 2024
@SaltyAom
Copy link
Member

Should be fixed in 1.0.0

Feels free to reopen if issue still persists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pending Should be resolved in next release
Projects
None yet
Development

No branches or pull requests

3 participants