E2B icon indicating copy to clipboard operation
E2B copied to clipboard

[E2B-456] Timeout doesn't work as expected in JS SDK

Open mlejva opened this issue 1 year ago • 3 comments

Describe the bug User reported this bug. The same problem happens with startAndWait()

The following code causes timeout after 60000ms:

// Build and evaluate the code
const procWithCustomHandler = await sandbox.process.start({
  cmd: "прm start",
  onStdout: (data) => console.log("process", data.line), 
  onStderr: (data) => console.log("process", data.line),
  timeout: 3 * 60 * 1000, // 3 minutes
});
await procWithCustomHandler.wait();

only when timeout is passed to wait() as well to .start() it starts working:

// Build and evaluate the code
const timeout = 3 * 60 * 1000; // 3 minutes
const procWithCustomHandler = await sandbox.process.start({
  cmd: "прm start",
  onStdout: (data) => console.log("process", data.line),
  onStderr: (data) = console.log("process", data.line),
  timeout
});

await procWithCustomHandler.wait(timeout);

E2B-456

mlejva avatar Dec 26 '23 08:12 mlejva

I've had a deeper look at the code, and I think I get the source of confusion:

The start and wait methods are completely separate, meaning they each have their own timeouts.

It seems a lot more likely that one would want to set a timeout for running the process, however and timeout passed to start (or startAndWait) is just for starting the process. The Python library does the same. Technically nothing is broken, but if you are using E2B for the first time this is going to be very confusing.

I would recommend potentially changing the naming convention, and at the very least making this distinction very clear in the docs.

jamesmurdza avatar Dec 27 '23 02:12 jamesmurdza

Hey @jamesmurdza, appreciate your feedback. We should look into it.

I would recommend potentially changing the naming convention, and at the very least making this distinction very clear in the docs. Any naming conventing that comes to your mind that would make this less confusing?

Thank you for the feedback!

mlejva avatar Dec 31 '23 11:12 mlejva

Maybe something like:

let procWithCustomHandler  = sandbox.process.start({ timeout: 60000 });
procWithCustomHandler.wait({ timeout: 300000 })

and

sandbox.process.startAndWait({ startTimeout: 60000, waitTimeout: 300000 });

or

sandbox.process.startAndWait({ timeout: 60000 }, { timeout: 300000 });

Or something along these lines.

jamesmurdza avatar Jan 03 '24 22:01 jamesmurdza

There are now two timeouts in the new beta SDK where it makes sense to separate the "start/request" timeout and the "wait" timeout:

  • https://e2b.dev/docs/guide/beta-migration#running-processes
  • https://e2b.dev/docs/guide/beta-migration#timeouts

ValentaTomas avatar Jul 22 '24 22:07 ValentaTomas