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

How to error if layout takes too long? #258

Open
keyserj opened this issue Jan 18, 2024 · 4 comments
Open

How to error if layout takes too long? #258

keyserj opened this issue Jan 18, 2024 · 4 comments

Comments

@keyserj
Copy link

keyserj commented Jan 18, 2024

Describe the bug
I've run into an infinite loop via eclipse/elk#993 and I'm trying to ensure my app can continue if a case like this turns up.

I initially thought I could use Promise.race with setTimeout to error:

const throwAfterTimeout = async <T>(timeout: number, promise: Promise<T>): Promise<T> => {
  const result = Promise.race([
    new Promise((_, reject) =>
      setTimeout(() => {
        reject(new Error("timeout"));
      }, timeout)
    ),
    promise,
  ]);
  return result as Promise<T>;
};

const layoutPromise = elk.layout(graph, {
  layoutOptions,
  logging: true,
  measureExecutionTime: true,
});

const layoutedGraph = await throwAfterTimeout(3000, layoutPromise);

but this still froze my app when laying out - I think I misunderstood how promises work, and that elk.layout must be reaching the infinite loop synchronously.

So then I looked more and realized, hey maybe that's what the out-of-the-box Web Worker support is for, since it seems that Web Workers are a way to run JavaScript on a separate thread. So I tried initializing elk to use a web worker (still with the above Promise.race implementation):

import ELK from "elkjs/lib/elk-api";
import { Worker as ElkWorker } from "elkjs/lib/elk-worker";

const elk = new ELK({
  workerFactory: (url) => new ElkWorker(url),
});

but my app still freezes when laying out.

Expected behavior
Can throw a timeout error after X seconds of trying to layout.

Screenshots

ELK Version
0.9.0 (which is 0.9.1 elkjs version?)

Additional context

I can't actually tell if the worker is being used, but no console errors are being thrown, and when I'm not using the layout that reproduces the infinite loop, layout is still performed as expected. before using the above Web Worker implementation, I did try the README's

import ELK from 'elkjs/lib/elk-api'
const elk = new ELK({
  workerUrl: './elk-worker.min.js'
})

but this results in Uncaught SyntaxError: Unexpected token '<' (at elk-worker.min.js:1:1). I can detail the various combinations I tried using to use a worker via workerUrl if we suspect that's the problem.

@keyserj keyserj added the bug label Jan 18, 2024
@zakhttp
Copy link

zakhttp commented Feb 15, 2024

Hey, wondering if there is an update on this ? or perhaps a around ?
Been facing the same issue for a while unfortunately :(

@zakhttp
Copy link

zakhttp commented Feb 16, 2024

Hey, wondering if there is an update on this ? or perhaps a around ? Been facing the same issue for a while unfortunately :(

I think that I found the problem here, it is an issue with create react app. CRA does not properly load the worker's script. The error signifies that the worker script cannot be found during runtime.
A dirty workaround would be to move elk-worker.min.js to the public folder and linking it like below works

const elk = new ELK({
    defaultLayoutOptions: options
    workerUrl: `${process.env.PUBLIC_URL}/elk-worker.min.js`,
});

However, a proper solution would be to take advantage of webpack 5 support of web workers

import ELK, { ElkExtendedEdge, ElkNode, LayoutOptions } from 'elkjs/lib/elk-api.js';

const elk = new ELK({
    defaultLayoutOptions: options,
    workerFactory: () => {
        return new Worker(
            /* webpackChunkName: "elk-worker.min" */ new URL('elkjs/lib/elk-worker.min.js', import.meta.url)
        );
    },
});

@soerendomroes
Copy link
Member

There is currently no process on our side. Is this something we should include in our readme?

@zakhttp
Copy link

zakhttp commented Feb 17, 2024

yeah that's a good idea I think. Could save others a ton of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants