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

fetchWebSocket should return a promise #286

Open
joepio opened this issue Jan 20, 2023 · 3 comments
Open

fetchWebSocket should return a promise #286

joepio opened this issue Jan 20, 2023 · 3 comments

Comments

@joepio
Copy link
Member

joepio commented Jan 20, 2023

Currenlty, fetchWebSocket simply sends a GET message. The received RESOURCE is then properly processed, but the function itself does not return this resource. That means users can't await the requested resource.

How can we fix this?

Well, we could add a .onmessage handler that calls the return thing, which is awaited. Or does this replace the earlier onmessage?

EDIT: Fixed, but we could need a more performant solution (see below)

@Polleps
Copy link
Member

Polleps commented Jan 20, 2023

For this to work the websocket functions have to become stateful. A WebsocketClient class with a map of currently requested resources and corresponding callback should do the trick

@joepio
Copy link
Member Author

joepio commented Jan 20, 2023

I've opted for something like this:

const defaultTimeout = 5000;

/** Sends a GET message for some resource over websockets. */
export async function fetchWebSocket(
  client: WebSocket,
  subject: string,
): Promise<Resource> {
  return new Promise((resolve, reject) => {
    client.addEventListener('message', function listener(ev) {
      const timeoutId = setTimeout(() => {
        client.removeEventListener('message', listener);
        reject(
          new Error(
            `Request for subject "${subject}" timed out after ${defaultTimeout}ms.`,
          ),
        );
      }, defaultTimeout);

      if (ev.data.startsWith('RESOURCE ')) {
        parseResourceMessage(ev).forEach(resource => {
          // if it is the requested subject, return the resource
          if (resource.getSubject() === subject) {
            clearTimeout(timeoutId);
            client.removeEventListener('message', listener);
            resolve(resource);
          }
        });
      }
    });
    client.send('GET ' + subject);
  });
}

joepio added a commit that referenced this issue Jan 20, 2023
@joepio
Copy link
Member Author

joepio commented Jan 20, 2023

I'm having some performance doupts.

Say on init, the user requests about 300 resources using websockets (e.g. when opening a Table or Chat or something).

For each incoming resource, the eventListeners for every other resource are called. This means that we have 300*300 evaluations.

I'm not sure if this will severely impact performance, but I can certainly imagine it will.

One simple way to prevent this, is by only doing the whole eventlistener thing if we know the requesting function is actually waiting for the response. If they aren't awaiting the response, we can simply ignore it.

But how do we know this?

  • In getResourceLoading (the most commonly used fetch-like function), we could pass an option noReturn or something. If this is present, we use a different function, like fetchWebsocketVoid

joepio added a commit that referenced this issue Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants