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

♻️ Refactor: added defaultmap + locking mechanism for intercepts queue #1441

Merged
merged 14 commits into from
Nov 29, 2023
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 51 additions & 15 deletions packages/core/src/network.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,50 @@ const ALLOWED_STATUSES = [200, 201, 301, 302, 304, 307, 308];
const ALLOWED_RESOURCES = ['Document', 'Stylesheet', 'Image', 'Media', 'Font', 'Other'];
const ABORTED_MESSAGE = 'Request was aborted by browser';

// DefaultMap, which returns a default value for an uninitialized key
// Similar to defaultDict in python
class DefaultMap extends Map {
nilshah98 marked this conversation as resolved.
Show resolved Hide resolved
constructor(getDefaultValue, ...mapConstructorArgs) {
super(...mapConstructorArgs);

if (typeof getDefaultValue !== 'function') {
throw new Error('getDefaultValue must be a function');
}

this.getDefaultValue = getDefaultValue;
}

get = (key) => {
if (!this.has(key)) {
this.set(key, this.getDefaultValue(key));
}

return super.get(key);
};
};

// RequestLifeCycleHandler handles life cycle of a requestId
// Ideal flow: requestWillBeSent -> requestPaused -> responseReceived -> loadingFinished / loadingFailed
// ServiceWorker flow: requestWillBeSent -> responseReceived -> loadingFinished / loadingFailed
class RequestLifeCycleHandler {
constructor() {
this.releaseRequest = null;
this.releaseResponse = null;
this.checkRequestSent = new Promise((resolve) => (this.releaseRequest = resolve));
this.checkResponseSent = new Promise((resolve) => (this.releaseResponse = resolve));
nilshah98 marked this conversation as resolved.
Show resolved Hide resolved
}
}
// The Interceptor class creates common handlers for dealing with intercepting asset requests
// for a given page using various devtools protocol events and commands.
export class Network {
static TIMEOUT = undefined;

log = logger('core:discovery');

#requestsLifeCycleHandler = new DefaultMap(() => new RequestLifeCycleHandler());
Copy link
Contributor

Choose a reason for hiding this comment

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

when is something deleted from this map ?
also considering we have this, we dont need any state sets right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when is something deleted from this map ?

Nothing is ever deleted from #requestsLifeCycleHandler since it is only used to manage lifecycles.
We can ideally delete requestId once all the locks are released from it's object, ie. it has reached end of lifecycle.
But, I don't see the point of it, as this is a very light map.

also considering we have this, we dont need any state sets right ?

Need #pending to communicate from requestWillBeSent to requestPaused states.
Can remove #intercepts completely.
Need #requests as it keeps mapping of all active requests and is what idle depends on.
Need #authentication as this keeps track of all requests that have already been authenticated.
Need #aborted to keep track of aborted requests.

In the current PR, I am planning to only remove #intercepts queue as it was used for communicating from requestPaused to requestWillBeSent but that should never happen now.

Copy link
Contributor

@ninadbstack ninadbstack Nov 24, 2023

Choose a reason for hiding this comment

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

as this is a very light map. do not fully agree with this as we could have thousands of requests in a single page load [ in fact we do for many storybook builds ] [ even though not blocking as it would be still few kbs max ]

_requestPaused could await requestWillBeSent, so it doesnt need to read from #pending as far as i understand. Because we are also storing event in pending with request id, you can just resolve with event for request will be sent
#requests agree we need it for now at least
#authentication could be done by checking if auth promise is already resolved [ but i dont mind using set either ]
#aborted is not necessary as you could just set aborted on #request object [ or in lifecycle like lifecyle.aborted ] it was done the way it is because we could process aborted event before requestWillBeSent and it wont exist in requests at all but as loadingFailed would await on requestWillBeSent we can be sure that request already exists in requests

Also other refactors are optional, not necessary in current pr

#pending = new Map();
#requests = new Map();
#intercepts = new Map();
// #intercepts = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

lets remove it entirely before merge

#authentications = new Set();
#aborted = new Set();

Expand Down Expand Up @@ -125,7 +159,7 @@ export class Network {

if (!keepPending) {
this.#pending.delete(requestId);
this.#intercepts.delete(requestId);
// this.#intercepts.delete(requestId);
}
}

Expand Down Expand Up @@ -153,16 +187,16 @@ export class Network {
// aborted. If the request is already pending, handle it; otherwise set it to be intercepted.
_handleRequestPaused = async (session, event) => {
let { networkId: requestId, requestId: interceptId, resourceType } = event;

// wait for request to be sent
await this.#requestsLifeCycleHandler.get(requestId).checkRequestSent;
let pending = this.#pending.get(requestId);
this.#pending.delete(requestId);

// guard against redirects with the same requestId
if (pending?.request.url === event.request.url &&
pending.request.method === event.request.method) {
await this._handleRequest(session, { ...pending, resourceType, interceptId });
} else {
// track the session that intercepted the request
this.#intercepts.set(requestId, { ...event, session });
}
}

Expand All @@ -175,15 +209,9 @@ export class Network {
if (request.url.startsWith('data:')) return;

if (this.intercept) {
let intercept = this.#intercepts.get(requestId);
this.#pending.set(requestId, event);

if (intercept) {
// handle the request with the session that intercepted it
let { session, requestId: interceptId, resourceType } = intercept;
await this._handleRequest(session, { ...event, resourceType, interceptId });
this.#intercepts.delete(requestId);
}
// release request
this.#requestsLifeCycleHandler.get(requestId).releaseRequest();
}
}

Expand Down Expand Up @@ -213,8 +241,11 @@ export class Network {

// Called when a response has been received for a specific request. Associates the response with
// the request data and adds a buffer method to fetch the response body when needed.
_handleResponseReceived = (session, event) => {
_handleResponseReceived = async (session, event) => {
let { requestId, response } = event;
// wait for upto 2 seconds or check if request has been sent
// no explicitly wait on requestWillBePaused as we implictly wait on it, since it manipulates the lifeCycle of request using Fetch module
await Promise.any([this.#requestsLifeCycleHandler.get(requestId).checkRequestSent, new Promise((resolve) => setTimeout(resolve, 2000))]);
let request = this.#requests.get(requestId);
/* istanbul ignore if: race condition paranioa */
if (!request) return;
Expand All @@ -224,6 +255,8 @@ export class Network {
let result = await this.send(session, 'Network.getResponseBody', { requestId });
return Buffer.from(result.body, result.base64Encoded ? 'base64' : 'utf-8');
};
// release response
this.#requestsLifeCycleHandler.get(requestId).releaseResponse();
}

// Called when a request streams events. These types of requests break asset discovery because
Expand All @@ -237,7 +270,10 @@ export class Network {
// Called when a request has finished loading which triggers the this.onrequestfinished
// callback. The request should have an associated response and be finished with any redirects.
_handleLoadingFinished = async event => {
let request = this.#requests.get(event.requestId);
let { requestId } = event;
// wait for upto 2 seconds or check if response has been sent
await Promise.any([this.#requestsLifeCycleHandler.get(requestId).checkResponseSent, new Promise((resolve) => setTimeout(resolve, 2000))]);
nilshah98 marked this conversation as resolved.
Show resolved Hide resolved
let request = this.#requests.get(requestId);
/* istanbul ignore if: race condition paranioa */
if (!request) return;

Expand Down
Loading