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.

Sign up for GitHub

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

Jump to bottom

fix: concurrency issue when running multiple requests #3442

Merged

gastonfournier merged 4 commits into main from frontend-concurrency-issue

Apr 4, 2023

Contributor

gastonfournier commented Apr 3, 2023 •

edited

About the changes

Running multiple calls to the frontend concurrently creates many instances of unleash SDK client.

The test added in commit 34004b5 is to verify we can reproduce the issue with a test case, and we can see the error here

The next commit introduces a fix for the issue

How to reproduce manually

Start Unleash (it has to be a fresh application: to retry the test, first restart Unleash)
Run:

#!/usr/bin/env bash

i=0
while [ $i -ne 15 ]
do
  curl http://localhost:4242/api/frontend -H 'Authorization: *:development.5487836a72f472d3494d92134fab6b4e07fcffe84c6c81a1d78448d0' &

  echo $i
  (( i=i+1 ));
done

You should see some errors, such as:

[2023-04-03T10:48:49.818] [ERROR] services/proxy-service.ts - Error: The unleash SDK has been initialized more than 10 times
    at /home/ivar/code/unleash/node_modules/unleash-client/lib/unleash.js:96:29


          test: validate the issue when running concurrent requests

34004b5

vercel bot commented Apr 3, 2023 •

edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 4, 2023 7:31am
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 4, 2023 7:31am


          Proxy code to solve the concurrency issue

72ef212

gastonfournier self-assigned this

gastonfournier added the bug label


          Add awaits

8baacc0

gastonfournier marked this pull request as ready for review

April 3, 2023 12:59

chriswk approved these changes

View reviewed changes

Contributor

chriswk left a comment

Great job Gaston. 👍

gastonfournier changed the title ~~test: validate the issue when running concurrent requests~~ fix: concurrency issue when running multiple requests

kwasniew reviewed

View reviewed changes

src/lib/services/proxy-service.ts

@@ @@ -42,7 +42,8 @@ export class ProxyService { @@
                   private readonly services: Services;
-                  private readonly clients: Map<ApiUser['secret'], Unleash> = new Map();
+                  private readonly clients: Map<ApiUser['secret'], Promise<Unleash>> =

Contributor

kwasniew Apr 3, 2023

It's weird that we store clients wrapped in promises instead of clients themselves. Why is that?

Contributor Author

gastonfournier Apr 3, 2023

Good question! And open to alternative solutions, but this is to store "we're processing/creating this unleash client, it might be ready soon", but we don't want to do that asynchronously as before because another request (or 15) might come while we're resolving the promise, and another client will be created. Ultimately, we want to know if someone is already processing a client for that secret, and wait until they finished before we proceed to create a new one

Member

ivarconr Apr 3, 2023

The challenge is that the current frontend API creates new node SDK instances on the fly for new tokens discovered.
Creating an SDK instance is an async operation, as it will require to fetch data to be ready.

This is probably not the best design, but fixing the bug at hand we need to make sure we await the client. I do not think there is a way to force that in to not being async, with the current implementation, where SDK instances are created on the fly.

In an optimal implementation of this, we should not use the SDK for other than the evaluation part. This would allow us to keep the caching and async part separate from the evaluation part.

src/lib/services/proxy-service.ts

		@@ -99,14 +100,13 @@ export class ProxyService {
		private async clientForProxyToken(token: ApiUser): Promise<Unleash> {

Contributor

kwasniew Apr 3, 2023

I'd expect that clientForProxyToken return a client not a promise, what's the reason for this decision?

Contributor Author

gastonfournier Apr 3, 2023

This is currently unchanged and I was lucky it was that way because it fits well with the change I did.
The reason for this is probably that we need to execute a client.start() here and that's async, which bubbles up until this line

src/lib/services/proxy-service.ts

-                          );
+                      let client = this.clients.get(token.secret);
+                      if (!client) {
+                          client = this.createClientForProxyToken(token);

Contributor

kwasniew Apr 3, 2023

if we awaited this one we could return client without the promise

Contributor Author

gastonfournier Apr 3, 2023

This causes the same problem as before (used the test to validate my assumption). The await will be executed in the event loop and meanwhile, another thread will execute clients.get, receive an empty response and yield a new createClientForProxyToken

src/lib/services/proxy-service.ts

+                  async deleteClientForProxyToken(secret: string): Promise<void> {
+                      let clientPromise = this.clients.get(secret);
+                      if (clientPromise) {
+                          const client = await clientPromise;

Contributor

kwasniew Apr 3, 2023

this is promise storage leaking out to this method. It would be easier to work with the client here

src/lib/services/proxy-service.ts

    
                  }

                  stopAll(): void {

                      this.clients.forEach((client) => client.destroy());

                      this.clients.forEach((promise) => promise.then((c) => c.destroy()));

Contributor

kwasniew Apr 3, 2023

same here, we pay the price of storing promises and not clients

Member

sighphyre commented Apr 3, 2023

@gastonfournier I think @kwasniew 's comments are on point here. I know why we went this way but my main worry is that some helpful person will read this code, decide this could be optimised and bring back the bug. If there's no cleaner way to do this, would a comment explaining why this code looks funny help prevent that in future?

Contributor Author

gastonfournier commented Apr 3, 2023

@gastonfournier I think @kwasniew 's comments are on point here. I know why we went this way but my main worry is that some helpful person will read this code, decide this could be optimised and bring back the bug. If there's no cleaner way to do this, would a comment explaining why this code looks funny help prevent that in future?

The test should help preventing this issue in the future, but I'll meet with @kwasniew tomorrow morning to look into alternative ways of doing this


          Add comment

f6baba6

Contributor Author

gastonfournier commented Apr 4, 2023

As we discussed, we're going to merge the PR and continue the conversation around improving this offline.

vercel bot deployed to Preview – unleash-monorepo-frontend

April 4, 2023 07:26

View deployment

vercel bot deployed to Preview – unleash-docs

April 4, 2023 07:31

View deployment

gastonfournier merged commit 2bfbe3c into main

14 checks passed

gastonfournier deleted the frontend-concurrency-issue branch

April 4, 2023 07:32

gastonfournier added a commit that referenced this pull request


          fix: concurrency issue when running multiple requests (#3442)

8c79b51

## About the changes

Fix issue when running multiple calls to the /frontend endpoint concurrently, which ends up creating many instances of unleash SDK client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment