-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fetchAuthSession is very slow on Angular SSR #14190
Comments
Hey, @martin-yumsto 👋 and thanks for opening this issue. Can you clarify if you're using the awsCredentials on the server side at all? And appreciate you linking the related Next.JS issue and fix. We'll look into this on the Angular side with SSR as well. |
Hi @martin-yumsto looking at your logs
Did you actually observe duplicate network calls within one |
Hello, appreciate you having a look into this as it was bothering me for a month now. I believe so, if I understand question well. My app is PWA and I wanted to keep it simple, so I don't make any exceptions for SSR and auth secured part of the code. Got this service that signs all the requests, my backend gateway of choice is AppSync which doesn't support real unauth access, so I need to sign requests for IAM anyway: export class AccessService {
private isPlatformBrowser = isPlatformBrowser(inject(PLATFORM_ID));
private cookies = extractCookiesFromRequest(
inject<Request | null>(REQUEST, { optional: true }),
);
accessHeaders(
request: HttpRequest<unknown>,
): Observable<Record<string, any>> {
return from(this.getAccessHeaders(request));
}
private async getServerAuthSession(): Promise<AuthSession> {
if (!this.cookies) {
return {};
}
return await runWithAmplifyServerContext(this.cookies, (contextSpec) =>
serverFetchAuthSession(contextSpec),
);
}
private getAuthSession(): Promise<AuthSession> {
return this.isPlatformBrowser
? fetchAuthSession()
: this.getServerAuthSession();
}
private async getAccessHeaders(request: HttpRequest<unknown>) {
let headers = request.headers
.keys()
.reduce((acc: Record<string, string>, key) => {
acc[key] = request.headers.get(key) || '';
return acc;
}, {});
try {
const { tokens, credentials } = await this.getAuthSession();
if (tokens) {
headers['Authorization'] = tokens.accessToken.toString();
return headers;
}
if (!credentials) {
return headers;
}
const signer = new SignatureV4({
service: 'appsync',
region: environment.awsRegion,
credentials: credentials,
sha256: Sha256,
});
const url = new URL(request.url);
const existingHeaders = request.headers.keys().reduce(
(acc, key) => {
acc[key] = request.headers.get(key) || '';
return acc;
},
{} as { [key: string]: string },
);
const signedRequest = await signer.sign({
method: request.method,
protocol: url.protocol,
headers: {
...existingHeaders,
'Content-Type': existingHeaders['Content-Type'] || 'application/json',
Host: url.host,
},
body: request.body ? JSON.stringify(request.body) : '',
hostname: url.hostname,
path: url.pathname,
});
return { ...headers, ...signedRequest.headers };
} catch {
return headers;
}
}
} |
Hi @HuiSF, I believe so, it's been over a month since I did original investigation and then was hoping to get some help on different discord servers. But nobody seemed to be able to help - so I'm now here. Let me double check tomorrow to be sure I wasn't hallucinating, however I'm very firm that single invoke of Here are my wrappers which I originally didn't post: export const cookieKeyValueStorage = (cookies: Record<string, string>) =>
createKeyValueStorageFromCookieStorageAdapter({
get: (name: string) => ({
name: name,
value: cookies?.[name],
}),
getAll: () =>
Object.entries(cookies ?? {}).map(([name, value]) => ({ name, value })),
set: (name: string, value: string) => {
// console.info('response headers: ', this.response?.headers)
// if(!this.response?.headers){
// return;
// }
// this.response.headers['Set-Cookie'] = `${name}=${value}`;
console.info('set', name, value);
// use framework cookie API to set a cookie
//
// you should implement this function if you need to update the
// token cookies on the client side from the server side
},
delete(name: string) {
console.info('delete', name);
// use framework cookies API to delete a cookie
},
});
import {
createUserPoolsTokenProvider,
createAWSCredentialsAndIdentityIdProvider,
runWithAmplifyServerContext as runAmplifyServerContext,
type AmplifyServer,
} from 'aws-amplify/adapter-core';
import { amplifyConfig } from '../auth-config';
import { cookieKeyValueStorage } from './cookie-key-value-storage';
export const runWithAmplifyServerContext = <T>(
cookies: Record<string, string>,
operation: (contextSpec: AmplifyServer.ContextSpec) => Promise<T>,
): Promise<T> => {
const cookieStorage = cookieKeyValueStorage(cookies);
return runAmplifyServerContext(
amplifyConfig,
{
Auth: {
tokenProvider: createUserPoolsTokenProvider(
amplifyConfig.Auth,
cookieStorage,
),
credentialsProvider: createAWSCredentialsAndIdentityIdProvider(
amplifyConfig.Auth,
cookieStorage,
),
},
},
operation,
);
}; EDIT: |
Thanks for the additional details @martin-yumsto re:
This was expected as when tokens are refreshed, the underlying token store interface that uses your I'll follow up once you confirm whether there are duplicate network calls made while calling |
indeed, it's Promise.all([
serverFetchAuthSession(contextSpec),
serverFetchUserAttributes(contextSpec),
]), that causes duplicate calls, each of them isolated causes following requests: serverFetchAuthSession:
serverFetchUserAttributes:
EDIT: That being said, calling them both simultaneously doesn't seem to cause the long processing. Calling them individually/isolated has the same times, arguably even worse, because I have seen 613ms for the first time.
That was expected from my side. What I witnessed was that cookie storage tried to perform same operation on the same cookie - e.g. delete on cookie named "A" multiple times. However I suppose it result of me calling both I'm still waiting for results of bumping to 6.12.3, I didn't face expired session yet. |
Hi @martin-yumsto thank you very much for providing additional details. Each If you need to call both const result = runWithAmplifyServerContext({
amplifyConfig,
{
Auth: {
tokenProvider: createUserPoolsTokenProvider(
amplifyConfig.Auth,
cookieStorage,
),
credentialsProvider: createAWSCredentialsAndIdentityIdProvider(
amplifyConfig.Auth,
cookieStorage,
),
},
},
async (contextSpect) => {
const session = await fetchAuthSession(contextSpec);
const attribtues = await fetchUserAttributes(contextSpec);
return { session, attributes };
},
}); Note that To make this work, you'd need to alter your key value store implementation, so your |
Hi @HuiSF, for unknown reasons times just got much better, it's still not amazing as it's now around 100-120ms to resolve. Didn't change any code or packages at all. Not sure if it's related to networking. I'll proceed and implement properly the delete and set cookie operations for the store. I suppose I need to implement some temporary one request caching on cookies as you mentioned, because result of the delete or set operations is new header on the response, but I guess get operation also expects to get those new values when again calling get. One question regarding your point - how crucial is it to only call My current implementation is that each API request during the SSR/CSR is signed using headers from I'd have to rethink my architecture and move it somewhere much higher in the design. Possibly creating one ServerContext and utilizing dependency injection to get it to all the places I need, that being said, it'll still be challenging not to call simultaneously |
Hi @martin-yumsto the latencies may depend on network conditions between your app server and Cognito service endpoints. The latency may be observable when the tokens need to be refreshed on the server side. In general, if you are aiming to perform multiple Amplify server-side APIs call within one request->response cycle, you should wrap all the API calls within one
|
Hi @HuiSF, I need to spent bit more time on it, did all the refactoringt o wrap my angular ssr with Moreover, I realized previously reported improvement in times was just due to accessing app using unauth session - with identity pool, when I switched to logged user with user pool today I'm back to 700ms. I'll keep you posted when I got single runAmplifyServerContext implemented and some progress. |
Thanks @martin-yumsto Re:
Also make sure you don't have |
I got bit confused, after trying everything and reading documentation and github lib code from top to bottom, left to right I'm still facing the issue of having the Amplify Server Context destroyed and I started to question my plans of introducing single It seems not even NextJs code does that, from the documentation it looks every invoke of server API is wrapped in it's own freshly created Can you please clarify if I understood you well that calling |
Hey @martin-yumsto there is no different how the underlying generic |
Hey @HuiSF, I made minimal reproduction repo to make it easier, this is post refactoring version that work locally, however, it doesn't work when I deploy it to lambda (still haven't figured why the amplify context gets destroyed before it's used). This is my output, you can see userInfo still takes really long to resolve:
EDIT: I created also repro for my first version which uses multiple
|
Hi @martin-yumsto thanks for providing the sample code repo. Which callsite that the the error "Attempted to get the Amplify Server Context that may have been destroyed." was thrown from precisely? Is that this This is function it seemed to use an "injected" In addition, looking at this runWithAmplifyServerContext({
amplifyConfig,
Auth: {},
async operation(contextSpec): {
// fetchAuthSession and fetchUserAttributes will be running in the same context since they
// are receiving the same contextSpec, so fetchUserAttributes() will use new tokens
// refreshed by fetchAuthSession()
const session = await fetchAuthSession(contextSpec);
const attrs = await fetchUserAttributes(contextSpect);
return [session, attrs];
}
}) |
thank you for taking a look so swiftly @HuiSF ! Just to make sure I understand your point well - my multiple I understand your point for the What would you recommend for that use case ? |
Hi @martin-yumsto looking at the branch Regarding:
Similarly, since your intercepting your GraphQL API globally by intercepting the underlying outgoing http request, you need to make sure the |
Before opening, please confirm:
JavaScript Framework
Angular
Amplify APIs
Authentication
Amplify Version
v6
Amplify Categories
auth
Backend
Other
Environment information
Describe the bug
I have noticed
fetchAuthSession
is very slow initially no matter the package - client/server. It's especially an issue on the SSR as it's purpose is to optimize for speed.It quite commonly takes around 300ms for the first (and only on the SSR)
fetchAuthSession
to get resolved. I noticed quite some calls are seemingly redundant and synchronous by monkey patching fetch:I have found #14079 and linked PR and issue which reports similar for NextJs. Unfortunately the fix seems to only address the issue for nextJs, as I'm on 6.12.0 and still face them.
Are there any plans to address the issue for lower level adapter which we need to use for Angular, too ? Or is my implementation likely wrong ?
Expected behavior
It should fetch fast
Reproduction steps
I only use Amplify Auth v6 gen 1 with backend deployed by terraform.
Framework is Angular SSR V19; I tried to remove all the Angular specific code, to make code snippet as small as possible
Code Snippet
Log output
aws-exports.js
Manual configuration
No response
Additional configuration
No response
Mobile Device
No response
Mobile Operating System
No response
Mobile Browser
No response
Mobile Browser Version
No response
Additional information and screenshots
No response
The text was updated successfully, but these errors were encountered: