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

Possible race condition in fetch setup #69

Open
RubenVerborgh opened this issue May 9, 2021 · 0 comments
Open

Possible race condition in fetch setup #69

RubenVerborgh opened this issue May 9, 2021 · 0 comments
Labels
bug Something isn't working

Comments

@RubenVerborgh
Copy link

The code at

constructor(
topic: string,
protocolList: protocols[],
options: BaseNotificationOptions = {}
) {
const { gateway, host, features = {}, fetch: fetchFn } = options;
this.topic = topic;
this.protocols = protocolList;
this.features = features;
this.gateway = gateway;
this.fetch = fetchFn || crossFetch;
// Attempt to load the fetch function from the default session if no fetchFn was passed in.
if (!fetchFn) {
// We don't care if this errors.
/* eslint @typescript-eslint/no-floating-promises: 0 */
BaseNotification.getDefaultSessionFetch().then(this.setSessionFetch);
}
this.host = host || BaseNotification.getRootDomain(topic);
}
setSessionFetch = (sessionFetch: typeof crossFetch = crossFetch): void => {
this.fetch = sessionFetch;
};

seems to suffer from 2 issues at first sight:

  • The fetch member is temporarily set to crossFetch and there is no indication of the switch happening. So when accessing the object quickly after creation, an unauthenticated fetch might accidentally be used.
  • There is no error handling. Even though the code says we "don't care" whether something errors (presumably because of the fallback to crossFetch), the promise rejection will still go unhandled and cause application errors/warnings. And I'd be surprised if we really don't care.

Both issues can be mitigated by temporarily assigning a function to this.fetch that stores all of its calls until the fetch object resolves. Something like:

const fetches = [];
this.fetch = (...args) => {
  return new Promise((reject, resolve) => {
    fetches.push({ args, reject, resolve });
  };
}

and then upon setting this.fetch, calling all fetches and resolve or reject depending on whether an error occurred or not.

This holding pattern can also be abstracted into a utility function.

@RubenVerborgh RubenVerborgh added the bug Something isn't working label May 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant