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

Allow requests to be modified in an async manner #4024

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/streaming/net/FetchLoader.js
Expand Up @@ -32,6 +32,7 @@
import FactoryMaker from '../../core/FactoryMaker';
import Settings from '../../core/Settings';
import Constants from '../constants/Constants';
import { modifyRequest } from '../utils/RequestModifier';

/**
* @module FetchLoader
Expand All @@ -54,7 +55,16 @@ function FetchLoader(cfg) {
}

function load(httpRequest) {
if (requestModifier && requestModifier.modifyRequest) {
modifyRequest(httpRequest, requestModifier)
.then(() => request(httpRequest));
}
else {
request(httpRequest);
}
}

function request(httpRequest) {
// Variables will be used in the callback functions
const requestStartTime = new Date();
const request = httpRequest.request;
Expand All @@ -77,7 +87,7 @@ function FetchLoader(cfg) {
request.requestStartDate = requestStartTime;
}

if (requestModifier) {
if (requestModifier && requestModifier.modifyRequestHeader) {
// modifyRequestHeader expects a XMLHttpRequest object so,
// to keep backward compatibility, we should expose a setRequestHeader method
// TODO: Remove RequestModifier dependency on XMLHttpRequest object and define
Expand Down Expand Up @@ -176,7 +186,7 @@ function FetchLoader(cfg) {
reader.read().then(function processFetch(args) {
const value = args.value;
const done = args.done;
markB = Date.now()
markB = Date.now();

if (value && value.length) {
const chunkDownloadDurationMS = markB - markA;
Expand Down
2 changes: 1 addition & 1 deletion src/streaming/net/HTTPLoader.js
Expand Up @@ -271,7 +271,7 @@ function HTTPLoader(cfg) {
}

let headers = null;
let modifiedUrl = requestModifier.modifyRequestURL(request.url);
let modifiedUrl = requestModifier.modifyRequestURL ? requestModifier.modifyRequestURL(request.url) : request.url;
littlespex marked this conversation as resolved.
Show resolved Hide resolved
if (settings.get().streaming.cmcd && settings.get().streaming.cmcd.enabled) {
const cmcdMode = settings.get().streaming.cmcd.mode;
if (cmcdMode === Constants.CMCD_MODE_QUERY) {
Expand Down
12 changes: 11 additions & 1 deletion src/streaming/net/XHRLoader.js
Expand Up @@ -29,6 +29,7 @@
* POSSIBILITY OF SUCH DAMAGE.
*/
import FactoryMaker from '../../core/FactoryMaker';
import { modifyRequest } from '../utils/RequestModifier';

/**
* @module XHRLoader
Expand All @@ -44,7 +45,16 @@ function XHRLoader(cfg) {
let instance;

function load(httpRequest) {
if (requestModifier && requestModifier.modifyRequest) {
modifyRequest(httpRequest, requestModifier)
.then(() => request(httpRequest));
}
else {
request(httpRequest);
}
}

function request(httpRequest) {
// Variables will be used in the callback functions
const requestStartTime = new Date();
const request = httpRequest.request;
Expand All @@ -64,7 +74,7 @@ function XHRLoader(cfg) {
request.requestStartDate = requestStartTime;
}

if (requestModifier) {
if (requestModifier && requestModifier.modifyRequestHeader) {
xhr = requestModifier.modifyRequestHeader(xhr, {
url: httpRequest.url
});
Expand Down
21 changes: 20 additions & 1 deletion src/streaming/utils/RequestModifier.js
Expand Up @@ -31,20 +31,39 @@

import FactoryMaker from '../../core/FactoryMaker';

export function modifyRequest(httpRequest, requestModifier) {
const request = {
url: httpRequest.url,
method: httpRequest.method,
headers: Object.assign({}, httpRequest.headers),
credentials: httpRequest.withCredentials ? 'include' : undefined,
dsilhavy marked this conversation as resolved.
Show resolved Hide resolved
};

return Promise.resolve(requestModifier.modifyRequest(request))
.then(() =>
Object.assign(httpRequest, request, { withCredentials: request.credentials === 'include' })
);
}

function RequestModifier() {

let instance;

function modifyRequest(request) {
return request;
}

function modifyRequestURL(url) {
return url;
}

// eslint-disable-next-line no-unused-vars
function modifyRequestHeader(request, {url}) {
function modifyRequestHeader(request, { url }) {
return request;
}

instance = {
modifyRequest: modifyRequest,
modifyRequestURL: modifyRequestURL,
modifyRequestHeader: modifyRequestHeader
};
Expand Down
35 changes: 16 additions & 19 deletions test/unit/dash.WebSegmentBaseLoader.js
Expand Up @@ -4,6 +4,7 @@ import EventBus from '../../src/core/EventBus';
import Events from '../../src/core/events/Events';
import Errors from '../../src/core/errors/Errors';
import RequestModifier from '../../src/streaming/utils/RequestModifier';
import { sleep } from './helpers/Async';

import ErrorHandlerMock from './mocks/ErrorHandlerMock';
import MediaPlayerModelMock from './mocks/MediaPlayerModelMock';
Expand Down Expand Up @@ -68,33 +69,29 @@ describe('WebmSegmentBaseLoader', function () {
webmSegmentBaseLoader.reset();
});

it('should trigger INITIALIZATION_LOADED event when loadInitialization function is called without representation parameter', function (done) {
it('should trigger INITIALIZATION_LOADED event when loadInitialization function is called without representation parameter', async function () {
const self = this.test.ctx;
webmSegmentBaseLoader.loadInitialization()
.then(() => {
done();
})
.catch((e) => {
done(e);
});

webmSegmentBaseLoader.loadInitialization();

await sleep(0);

self.requests[0].respond(200);
});

it('should trigger SEGMENTS_LOADED event with an error when loadSegments function is called without representation parameter', function (done) {
it('should trigger SEGMENTS_LOADED event with an error when loadSegments function is called without representation parameter', async function () {
const self = this.test.ctx;

webmSegmentBaseLoader.loadSegments()
.then((e) => {
expect(e.error).not.to.equal(undefined);
expect(e.error.code).to.equal(Errors.SEGMENT_BASE_LOADER_ERROR_CODE);
expect(e.error.message).to.equal(Errors.SEGMENT_BASE_LOADER_ERROR_MESSAGE);
done();
})
.catch((e) => {
done(e);
});
const promise = webmSegmentBaseLoader.loadSegments();

await sleep(0);

self.requests[0].respond(200);

const e = await promise;
expect(e.error).not.to.equal(undefined);
expect(e.error.code).to.equal(Errors.SEGMENT_BASE_LOADER_ERROR_CODE);
expect(e.error.message).to.equal(Errors.SEGMENT_BASE_LOADER_ERROR_MESSAGE);
});
});
});
3 changes: 3 additions & 0 deletions test/unit/helpers/Async.js
@@ -0,0 +1,3 @@
export function sleep(ms) {
return new Promise(resolve => setTimeout(resolve, ms));
}
21 changes: 17 additions & 4 deletions test/unit/streaming.net.HTTPLoader.js
Expand Up @@ -9,6 +9,7 @@ import {
}
from '../../src/streaming/vo/metrics/HTTPRequest';
import Settings from '../../src/core/Settings';
import { sleep } from './helpers/Async';

const expect = require('chai').expect;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test should be added to verify that an optional modifyRequestURL function is supported. Once this ships and people are supplying a requestModifer without this method it shouldn't regress.

const sinon = require('sinon');
Expand Down Expand Up @@ -105,7 +106,7 @@ describe('HTTPLoader', function () {
expect(httpLoader.load.bind(httpLoader, { request: {} })).to.throw('config object is not correct or missing');
});

it('should use XHRLoader if it is not an arraybuffer request even if availabilityTimeComplete is set to false', () => {
it('should use XHRLoader if it is not an arraybuffer request even if availabilityTimeComplete is set to false', async () => {
let self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -128,12 +129,15 @@ describe('HTTPLoader', function () {
availabilityTimeComplete: false
}, success: callbackSucceeded, complete: callbackCompleted, error: callbackError
});

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].respond(200);
sinon.assert.notCalled(global.fetch);
});

it('should use XHRLoader and call success and complete callback when load is called successfully', () => {
it('should use XHRLoader and call success and complete callback when load is called successfully', async () => {
let self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -154,6 +158,9 @@ describe('HTTPLoader', function () {
complete: callbackCompleted,
error: callbackError
});

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].respond(200);
sinon.assert.notCalled(global.fetch);
Expand All @@ -162,7 +169,7 @@ describe('HTTPLoader', function () {
expect(callbackSucceeded.calledBefore(callbackCompleted)).to.be.true; // jshint ignore:line
});

it('should use XHRLoader and call error and complete callback when load is called with error', () => {
it('should use XHRLoader and call error and complete callback when load is called with error', async () => {
let self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -183,6 +190,9 @@ describe('HTTPLoader', function () {
complete: callbackCompleted,
error: callbackError
});

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].respond(404);
sinon.assert.calledOnce(callbackError);
Expand All @@ -191,7 +201,7 @@ describe('HTTPLoader', function () {
expect(callbackError.calledBefore(callbackCompleted)).to.be.true; // jshint ignore:line
});

it('should use XHRLoader if it is not a MEDIA_SEGMENT_TYPE request even if availabilityTimeComplete is set to false and it is an arraybuffer request', () => {
it('should use XHRLoader if it is not a MEDIA_SEGMENT_TYPE request even if availabilityTimeComplete is set to false and it is an arraybuffer request', async () => {
let self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -214,6 +224,9 @@ describe('HTTPLoader', function () {
availabilityTimeComplete: false
}, success: callbackSucceeded, complete: callbackCompleted, error: callbackError
});

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].respond(200);
sinon.assert.notCalled(global.fetch);
Expand Down
26 changes: 21 additions & 5 deletions test/unit/streaming.net.XHRLoader.js
@@ -1,5 +1,6 @@
import XHRLoader from '../../src/streaming/net/XHRLoader';
import RequestModifier from '../../src/streaming/utils/RequestModifier';
import { sleep } from './helpers/Async';

const expect = require('chai').expect;
const sinon = require('sinon');
Expand Down Expand Up @@ -31,7 +32,7 @@ describe('XHRLoader', function () {
afterEach(function () {
});

it('should call success and complete callback when load is called successfully', () => {
it('should call success and complete callback when load is called successfully', async () => {
const self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -51,6 +52,9 @@ describe('XHRLoader', function () {
onabort: callbackAbort
};
xhrLoader.load(request);

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].respond(200);
sinon.assert.notCalled(callbackError);
Expand All @@ -60,7 +64,7 @@ describe('XHRLoader', function () {
expect(callbackSucceeded.calledBefore(callbackCompleted)).to.be.true; // jshint ignore:line
});

it('should call onload and complete callback when load is called and there is an error response', () => {
it('should call onload and complete callback when load is called and there is an error response', async () => {
const self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -79,6 +83,9 @@ describe('XHRLoader', function () {
onabort: callbackAbort
};
xhrLoader.load(request);

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].respond(404);
sinon.assert.notCalled(callbackError);
Expand All @@ -88,7 +95,7 @@ describe('XHRLoader', function () {
expect(callbackSucceeded.calledBefore(callbackCompleted)).to.be.true; // jshint ignore:line
});

it('should call onabort callback when abort is called', () => {
it('should call onabort callback when abort is called', async () => {
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
const callbackError = sinon.spy();
Expand All @@ -108,6 +115,9 @@ describe('XHRLoader', function () {
onabort: callbackAbort
};
xhrLoader.load(request);

await sleep(0);

xhrLoader.abort(request);

sinon.assert.notCalled(callbackError);
Expand All @@ -117,7 +127,7 @@ describe('XHRLoader', function () {
sinon.assert.calledOnce(callbackAbort);
});

it('should call onerror and onend when load is called and there is a network error', () => {
it('should call onerror and onend when load is called and there is a network error', async () => {
const self = this.ctx;
const callbackSucceeded = sinon.spy();
const callbackCompleted = sinon.spy();
Expand All @@ -136,6 +146,9 @@ describe('XHRLoader', function () {
onabort: callbackAbort
};
xhrLoader.load(request);

await sleep(0);

expect(self.requests.length).to.equal(1);
self.requests[0].error();
sinon.assert.calledOnce(callbackError);
Expand All @@ -145,7 +158,7 @@ describe('XHRLoader', function () {
expect(callbackError.calledBefore(callbackCompleted)).to.be.true; // jshint ignore:line
});

it('should set timeout on the sending XHR request', () => {
it('should set timeout on the sending XHR request', async () => {
xhrLoader = XHRLoader(context).create({
requestModifier: requestModifier
});
Expand All @@ -156,6 +169,9 @@ describe('XHRLoader', function () {
timeout: 100
};
xhrLoader.load(request);

await sleep(0);

expect(request.response.timeout).to.be.equal(100);
});
});