Skip to content

Commit

Permalink
[Fix][DEVX-461] Concurrency issue in TypeScript SDK (#807)
Browse files Browse the repository at this point in the history
* fix(sdk-client-v3): fix concurrency issue in sdk

- add the async-mutex to sync concurrent executions

* chore(sdk-client-v3): remove comments

- remove commented code block
  • Loading branch information
ajimae authored Sep 18, 2024
1 parent b8489ba commit dfbe4ce
Show file tree
Hide file tree
Showing 8 changed files with 62 additions and 35 deletions.
5 changes: 5 additions & 0 deletions .changeset/tidy-bears-swim.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@commercetools/ts-client': patch
---

Fix concurrency issues in TypeScript v3 SDK
25 changes: 13 additions & 12 deletions packages/sdk-client-v3/src/client/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,18 +166,19 @@ export default function createClient(middlewares: ClientOptions): Client {
process,
execute(request: ClientRequest): Promise<ClientResult> {
validate('exec', request)
return new Promise((resolve, reject) => {
dispatch({ reject, resolve, ...request })
.then((res) => {
if (res.error) return reject(res.error)

if (res.originalRequest && _maskSensitiveHeaderData) {
res.originalRequest = maskAuthData(res.originalRequest)
}

resolve(res)
})
.catch(reject)
return new Promise(async (resolve, reject) => {
try {
const response = await dispatch({ reject, resolve, ...request })
if (response.error) return reject(response.error)

if (response.originalRequest && _maskSensitiveHeaderData) {
response.originalRequest = maskAuthData(response.originalRequest)
}

resolve(response)
} catch (err) {
reject(err)
}
})
},
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Mutex } from 'async-mutex'
import fetch from 'node-fetch'
import {
AuthMiddlewareOptions,
ClientRequest,
Middleware,
MiddlewareRequest,
MiddlewareResponse,
Next,
RequestState,
RequestStateStore,
Task,
TokenCache,
TokenStore,
Expand All @@ -19,7 +19,7 @@ export default function createAuthMiddlewareForAnonymousSessionFlow(
options: AuthMiddlewareOptions
): Middleware {
const pendingTasks: Array<Task> = []
const requestState = store<RequestState, RequestStateStore>(false)
const requestState = new Mutex()
const tokenCache =
options.tokenCache ||
store<TokenStore, TokenCache>({
Expand Down Expand Up @@ -54,7 +54,14 @@ export default function createAuthMiddlewareForAnonymousSessionFlow(
}

// make request to coco
const requestWithAuth = await executeRequest(requestOptions)
let requestWithAuth: ClientRequest

try {
await requestState.acquire()
requestWithAuth = await executeRequest(requestOptions)
} finally {
requestState.release()
}

if (requestWithAuth) {
return next(requestWithAuth)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Mutex } from 'async-mutex'
import fetch from 'node-fetch'
import {
ClientRequest,
Middleware,
MiddlewareRequest,
MiddlewareResponse,
Next,
PasswordAuthMiddlewareOptions,
RequestState,
RequestStateStore,
Task,
} from '../../types/types'
import { buildTokenCacheKey, store } from '../../utils'
Expand All @@ -24,7 +24,7 @@ export default function createAuthMiddlewareForPasswordFlow(
})

const pendingTasks: Array<Task> = []
const requestState = store<RequestState, RequestStateStore>(false)
const requestState = new Mutex()

const tokenCacheKey = buildTokenCacheKey(options)

Expand All @@ -50,7 +50,15 @@ export default function createAuthMiddlewareForPasswordFlow(
}

// make request to coco
const requestWithAuth = await executeRequest(requestOptions)
let requestWithAuth: ClientRequest

try {
await requestState.acquire()
requestWithAuth = await executeRequest(requestOptions)
} finally {
requestState.release()
}

if (requestWithAuth) {
return next(requestWithAuth)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { Mutex } from 'async-mutex'
import {
ClientRequest,
Middleware,
MiddlewareRequest,
MiddlewareResponse,
Next,
RefreshAuthMiddlewareOptions,
RequestState,
RequestStateStore,
Task,
} from '../../types/types'
import { store } from '../../utils'
Expand All @@ -23,7 +23,7 @@ export default function createAuthMiddlewareForRefreshTokenFlow(
})

const pendingTasks: Array<Task> = []
const requestState = store<RequestState, RequestStateStore>(false)
const requestState = new Mutex()

return (next: Next) => {
return async (request: MiddlewareRequest): Promise<MiddlewareResponse> => {
Expand All @@ -46,7 +46,15 @@ export default function createAuthMiddlewareForRefreshTokenFlow(
}

// make request to coco
const requestWithAuth = await executeRequest(requestOptions)
let requestWithAuth: ClientRequest

try {
await requestState.acquire()
requestWithAuth = await executeRequest(requestOptions)
} finally {
requestState.release()
}

if (requestWithAuth) {
return next(requestWithAuth)
}
Expand Down
4 changes: 1 addition & 3 deletions packages/sdk-client-v3/src/types/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,16 +142,14 @@ export type RefreshAuthMiddlewareOptions = {
httpClient?: Function
}

export type RequestStateStore = any

/* Request */
type requestBaseOptions = {
url: string
body: string
basicAuth: string
request: MiddlewareRequest
tokenCache: TokenCache,
requestState: RequestStateStore,
requestState: unknown,
pendingTasks: Array<Task>,
tokenCacheKey?: TokenCacheOptions,
}
Expand Down
10 changes: 5 additions & 5 deletions packages/sdk-client-v3/tests/auth/anonymous-flow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ function createTestMiddlewareOptions(options) {
describe('Anonymous Session Flow', () => {
describe('Anonymous session flow', () => {
test('should throw if `options` are not provided', () => {
const middlewareOptions = null
const middlewareOptions: any = null

expect(() =>
buildRequestForAnonymousSessionFlow(middlewareOptions)
Expand Down Expand Up @@ -202,7 +202,7 @@ describe('Anonymous Session Flow', () => {
}))

test('should not throw if `anonymousId` options is not provided.', () =>
new Promise((resolve, reject) => {
new Promise(async (resolve, reject) => {
const response = createTestResponse({
resolve,
reject,
Expand Down Expand Up @@ -232,9 +232,9 @@ describe('Anonymous Session Flow', () => {
},
})

createAuthMiddlewareForAnonymousSessionFlow(middlewareOptions)(next)(
createTestRequest({})
)
await createAuthMiddlewareForAnonymousSessionFlow(middlewareOptions)(
next
)(createTestRequest({}))

expect(middlewareOptions.httpClient).toHaveBeenCalled()
expect(middlewareOptions.httpClient).toHaveBeenCalledTimes(1)
Expand Down
6 changes: 3 additions & 3 deletions packages/sdk-client-v3/tests/auth/refresh-token.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('Refresh Token Flow', () => {
describe('Refresh token flow auth request builder', () => {
test('should throw if `options` are not provided.', () => {
new Promise((resolve, reject) => {
const middlewareOptions = null
const middlewareOptions: any = null
expect(() =>
buildRequestForRefreshTokenFlow(middlewareOptions)
).toThrow('Missing required options')
Expand Down Expand Up @@ -269,7 +269,7 @@ describe('Refresh Token Flow', () => {
}))

test('should fetch and store token in tokenCache object', () =>
new Promise((resolve, reject) => {
new Promise(async (resolve, reject) => {
const response = createTestResponse({
resolve,
reject,
Expand Down Expand Up @@ -314,7 +314,7 @@ describe('Refresh Token Flow', () => {
tokenCache,
})

createAuthMiddlewareForRefreshTokenFlow(middlewareOptions)(next)(
await createAuthMiddlewareForRefreshTokenFlow(middlewareOptions)(next)(
createTestRequest({})
)
expect(middlewareOptions.httpClient).toHaveBeenCalledTimes(1)
Expand Down

0 comments on commit dfbe4ce

Please sign in to comment.