From abdcc5942575a9126f63d47a0bb93a93961b65ab Mon Sep 17 00:00:00 2001
From: Guy Harwood <GuyHarwood@users.noreply.github.com>
Date: Mon, 12 Jun 2023 16:27:16 +0100
Subject: [PATCH] feature/59309 change default sas timeout for pupil submission
tokens (#2533)
* change default sas timeout for pupil submission tokens
* increase retry count for check submission
* add exponential retry config to polly for check submission
* remove debug output
---------
Co-authored-by: Jon Shipley <jon-shipley@users.noreply.github.com>
---
admin/config.js | 2 +-
pupil-spa/gen_config.sh | 2 +-
.../app/services/http/http.service.spec.ts | 485 ++++++++++--------
.../src/app/services/http/http.service.ts | 11 +-
4 files changed, 270 insertions(+), 230 deletions(-)
diff --git a/admin/config.js b/admin/config.js
index 2d1639e01e..403603ed72 100644
--- a/admin/config.js
+++ b/admin/config.js
@@ -125,7 +125,7 @@ module.exports = {
Tokens: {
// 12 hours default expiry
jwtTimeOutHours: process.env.JWT_TIMEOUT_HOURS || 12,
- sasTimeOutHours: process.env.SAS_TIMEOUT_HOURS || 25
+ sasTimeOutHours: process.env.SAS_TIMEOUT_HOURS || 120
},
Monitoring: {
ApplicationInsights: {
diff --git a/pupil-spa/gen_config.sh b/pupil-spa/gen_config.sh
index dbadcc72fc..16e2102662 100755
--- a/pupil-spa/gen_config.sh
+++ b/pupil-spa/gen_config.sh
@@ -12,7 +12,7 @@ authUrl=${AUTH_URL:-"http://localhost:3003/auth"}
checkStartAPIErrorDelay=${CHECK_START_ERROR_DELAY:-"2000"}
checkStartAPIErrorMaxAttempts=${CHECK_START_MAX_ATTEMPTS:-"3"}
checkSubmissionAPIErrorDelay=${CHECK_SUBMISSION_ERROR_DELAY:-"30000"}
-checkSubmissionAPIErrorMaxAttempts=${CHECK_SUBMISSION_MAX_ATTEMPTS:-"3"}
+checkSubmissionAPIErrorMaxAttempts=${CHECK_SUBMISSION_MAX_ATTEMPTS:-"10"}
connectivityCheckEnabled=${CONNECTIVITY_CHECK_ENABLED:-"false"}
connectivityCheckViewMinDisplay=${CONNECTIVITY_CHECK_MIN_DISPLAY:-"6000"}
feedbackAPIErrorDelay=${CHECK_SUBMISSION_ERROR_DELAY:-"3000"}
diff --git a/pupil-spa/src/app/services/http/http.service.spec.ts b/pupil-spa/src/app/services/http/http.service.spec.ts
index c98015acab..d7b92c4b2f 100644
--- a/pupil-spa/src/app/services/http/http.service.spec.ts
+++ b/pupil-spa/src/app/services/http/http.service.spec.ts
@@ -1,23 +1,23 @@
-import { TestBed } from '@angular/core/testing';
-import { HttpService } from './http.service';
-import { HttpErrorResponse, HttpHeaders } from '@angular/common/http';
-import { HttpClientTestingModule } from '@angular/common/http/testing';
-import { defer } from 'rxjs/internal/observable/defer';
+import { TestBed } from '@angular/core/testing'
+import { HttpService } from './http.service'
+import { HttpErrorResponse, HttpHeaders } from '@angular/common/http'
+import { HttpClientTestingModule } from '@angular/common/http/testing'
+import { defer } from 'rxjs/internal/observable/defer'
/** Create async observable that emits-once and completes
* after a JS engine turn */
-export function asyncData<T>(data: T) {
- return defer(() => Promise.resolve(data));
+export function asyncData<T> (data: T) {
+ return defer(() => Promise.resolve(data))
}
/** Create async observable that emits-once and rejects
* after a JS engine turn */
-export function asyncError<T>(data: T) {
- return defer(() => Promise.reject(data));
+export function asyncError<T> (data: T) {
+ return defer(() => Promise.reject(data))
}
describe('HttpService', () => {
- let httpClientSpy: { get: jasmine.Spy, post: jasmine.Spy };
- let httpService: HttpService;
+ let httpClientSpy: { get: jasmine.Spy, post: jasmine.Spy, request: jasmine.Spy }
+ let httpService: HttpService
beforeEach(() => {
TestBed.configureTestingModule({
@@ -25,222 +25,255 @@ describe('HttpService', () => {
providers: [
HttpService
]
- });
+ })
// Here is how to construct an actual HttpClient instance without DI
// service = new HttpService(new HttpClient(new HttpXhrBackend({ build: () => new XMLHttpRequest() })));
- httpClientSpy = jasmine.createSpyObj('HttpClient', ['get', 'post']);
- httpService = new HttpService(httpClientSpy as any);
- });
+ httpClientSpy = jasmine.createSpyObj('HttpClient', ['get', 'post', 'request'])
+ httpService = new HttpService(httpClientSpy as any)
+ })
it('should be created', () => {
- expect(httpService).toBeTruthy();
- });
-
- it('returns the http result from a post on success', async () => {
- try {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and.returnValue(asyncData(expectedResponse));
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(1);
- } catch (error) {
- fail(error);
- }
- });
-
- it('returns the httpErrorResponse from a post on 401 unauthorised, single call only', async () => {
- try {
- httpClientSpy.post.and.returnValue(asyncError(new HttpErrorResponse({
- error: { error: 'Unauthorised' },
- status: 401,
- statusText: 'Unauthorized'
- })));
- await httpService.postJson('http://localhost', { foo: 'bar' });
- fail('Should have thrown');
- } catch (error) {
- expect(error.status).toBe(401);
- expect(error.statusText).toBe('Unauthorized');
- expect(error.error).toEqual({ error: 'Unauthorised' });
- expect(httpClientSpy.post).toHaveBeenCalledTimes(1);
- }
- });
-
- it('retries if the error code is 408 - Request Timeout', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: 'Request Timeout',
- status: 408,
- statusText: 'Request Timeout'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-
- it('retries if the error code is 429 - Too many requests', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: 'Too many requests',
- status: 429,
- statusText: 'Too many requests'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-
- it('retries if the error code is 500 - Server Error', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: 'Server error',
- status: 500,
- statusText: 'Server error'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-
- it('retries if the error code is 502 - Bad gateway', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: 'Bad gateway',
- status: 502,
- statusText: 'Bad gateway'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-
- it('retries if the error code is 503 - Service unavailable', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: 'Service unavailable',
- status: 503,
- statusText: 'Service unavailable'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-
- it('retries if the error code is 504 - Gateway timeout', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: 'Gateway timeout',
- status: 504,
- statusText: 'Gateway timeout'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-
- it('retries 5 times', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // initial request
- asyncError(new HttpErrorResponse({
- error: 'Gateway timeout',
- status: 504,
- statusText: 'Gateway timeout',
- headers: new HttpHeaders({ 'X-jms': (new Date()).toUTCString() })
- })),
- // 1st retry
- asyncError(new HttpErrorResponse({
- error: 'Server error',
- status: 500,
- statusText: 'Server error'
- })),
- // 2nd retry
- asyncError(new HttpErrorResponse({
- error: 'Service Unavailable',
- status: 503,
- statusText: 'Service Unavailable'
- })),
- // 3rd retry
- asyncError(new HttpErrorResponse({
- error: 'Request timeout',
- status: 408,
- statusText: 'Request timeout'
- })),
- // 4th retry
- asyncError(new HttpErrorResponse({
- error: 'Bad gateway',
- status: 502,
- statusText: 'Bad gateway'
- })),
- // 5th retry
- asyncData(expectedResponse)
- );
- const startTime = Date.now();
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- const endTime = Date.now();
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(6);
- // Retry timings are:
- // original call: 0
- // call 1: +100ms
- // call 2: +200
- // call 3: +400
- // call 4: +800
- // call 5: +1600
- expect(endTime - startTime).toBeGreaterThanOrEqual(1600 + 800 + 400 + 200 + 100);
- });
-
- it('retries if there is a client side error', async () => {
- const expectedResponse = { checkCode: '1111-2222-3333-4444' };
- httpClientSpy.post.and
- .returnValues(
- // 1st response
- asyncError(new HttpErrorResponse({
- error: new ErrorEvent('Connect Failure'),
- status: 400,
- statusText: 'Connect Failure'
- })),
- // 2nd response
- asyncData(expectedResponse)
- );
- const res = await httpService.postJson('http://localhost', { foo: 'bar' });
- expect(res.checkCode).toBeTruthy();
- expect(httpClientSpy.post).toHaveBeenCalledTimes(2);
- });
-}); // end describe
+ expect(httpService).toBeTruthy()
+ })
+
+ describe('postJson', () => {
+ it('returns the http result from a post on success', async () => {
+ try {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and.returnValue(asyncData(expectedResponse))
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(1)
+ } catch (error) {
+ fail(error)
+ }
+ })
+
+ it('returns the httpErrorResponse from a post on 401 unauthorised, single call only', async () => {
+ try {
+ httpClientSpy.post.and.returnValue(asyncError(new HttpErrorResponse({
+ error: { error: 'Unauthorised' },
+ status: 401,
+ statusText: 'Unauthorized'
+ })))
+ await httpService.postJson('http://localhost', { foo: 'bar' })
+ fail('Should have thrown')
+ } catch (error) {
+ expect(error.status).toBe(401)
+ expect(error.statusText).toBe('Unauthorized')
+ expect(error.error).toEqual({ error: 'Unauthorised' })
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(1)
+ }
+ })
+
+ it('retries if the error code is 408 - Request Timeout', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: 'Request Timeout',
+ status: 408,
+ statusText: 'Request Timeout'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ it('retries if the error code is 429 - Too many requests', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: 'Too many requests',
+ status: 429,
+ statusText: 'Too many requests'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ it('retries if the error code is 500 - Server Error', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: 'Server error',
+ status: 500,
+ statusText: 'Server error'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ it('retries if the error code is 502 - Bad gateway', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: 'Bad gateway',
+ status: 502,
+ statusText: 'Bad gateway'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ it('retries if the error code is 503 - Service unavailable', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: 'Service unavailable',
+ status: 503,
+ statusText: 'Service unavailable'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ it('retries if the error code is 504 - Gateway timeout', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: 'Gateway timeout',
+ status: 504,
+ statusText: 'Gateway timeout'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ it('retries 5 times', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // initial request
+ asyncError(new HttpErrorResponse({
+ error: 'Gateway timeout',
+ status: 504,
+ statusText: 'Gateway timeout',
+ headers: new HttpHeaders({ 'X-jms': (new Date()).toUTCString() })
+ })),
+ // 1st retry
+ asyncError(new HttpErrorResponse({
+ error: 'Server error',
+ status: 500,
+ statusText: 'Server error'
+ })),
+ // 2nd retry
+ asyncError(new HttpErrorResponse({
+ error: 'Service Unavailable',
+ status: 503,
+ statusText: 'Service Unavailable'
+ })),
+ // 3rd retry
+ asyncError(new HttpErrorResponse({
+ error: 'Request timeout',
+ status: 408,
+ statusText: 'Request timeout'
+ })),
+ // 4th retry
+ asyncError(new HttpErrorResponse({
+ error: 'Bad gateway',
+ status: 502,
+ statusText: 'Bad gateway'
+ })),
+ // 5th retry
+ asyncData(expectedResponse)
+ )
+ const startTime = Date.now()
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ const endTime = Date.now()
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(6)
+ // Retry timings are:
+ // original call: 0
+ // call 1: +100ms
+ // call 2: +200
+ // call 3: +400
+ // call 4: +800
+ // call 5: +1600
+ expect(endTime - startTime).toBeGreaterThanOrEqual(1600 + 800 + 400 + 200 + 100)
+ })
+
+ it('retries if there is a client side error', async () => {
+ const expectedResponse = { checkCode: '1111-2222-3333-4444' }
+ httpClientSpy.post.and
+ .returnValues(
+ // 1st response
+ asyncError(new HttpErrorResponse({
+ error: new ErrorEvent('Connect Failure'),
+ status: 400,
+ statusText: 'Connect Failure'
+ })),
+ // 2nd response
+ asyncData(expectedResponse)
+ )
+ const res = await httpService.postJson('http://localhost', { foo: 'bar' })
+ expect(res.checkCode).toBeTruthy()
+ expect(httpClientSpy.post).toHaveBeenCalledTimes(2)
+ })
+
+ })
+
+ describe('postXml', () => {
+ const originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL
+ beforeAll(() => {
+ jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000
+ })
+
+ afterAll(() => {
+ jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout
+ })
+
+ it('tries the default number of times with exponential backoff', async () => {
+ httpClientSpy.request.and
+ .returnValue(
+ asyncError(new HttpErrorResponse({
+ error: 'Server error',
+ status: 500,
+ statusText: 'Server error'
+ })),
+ )
+ const retryCount = 3;
+ try {
+ await httpService.postXml('http://localhost', '<xml></xml>', new HttpHeaders(), retryCount)
+ fail('Should have thrown')
+ } catch (error) {
+ expect(httpClientSpy.request).toHaveBeenCalledTimes(retryCount + 1);
+ }
+ })
+
+ }) // end postXml describe
+}) // end main describe
diff --git a/pupil-spa/src/app/services/http/http.service.ts b/pupil-spa/src/app/services/http/http.service.ts
index 85dfd0967f..7fd9313433 100644
--- a/pupil-spa/src/app/services/http/http.service.ts
+++ b/pupil-spa/src/app/services/http/http.service.ts
@@ -44,7 +44,14 @@ export class HttpService {
public async postXml(url: string, body: any, headers: HttpHeaders, retryCount = this.defaultRetryCount): Promise<any> {
const _that = this;
- _that.config.retryTimes = retryCount;
+ const retryConfig = new Array<number>();
+ const baseDelay = 3000;
+ const increaseFactor = 1.5;
+ retryConfig.push(baseDelay);
+ for (let i = 1; i < retryCount; i++) {
+ const instanceDelay = Math.round(increaseFactor * retryConfig[i - 1]);
+ retryConfig.push(instanceDelay);
+ }
const response = await polly()
.handle(error => {
// Any requests that don't give a 200 status code will raise an Exception that can be handled here.
@@ -86,7 +93,7 @@ export class HttpService {
console.warn('http-service: error: unusual error response (will not retry):', error);
return false; // shouldn't get here
})
- .waitAndRetry(_that.config.retryTimes)
+ .waitAndRetry(retryConfig)
.executeForPromise(function () {
return _that.http.request('POST', url, {
headers: headers.set('Content-Type', 'text/xml'),