e2b-dev / E2B

Secure open source cloud runtime for AI apps & AI agents
https://e2b.dev/docs
Apache License 2.0
7.02k stars 458 forks source link

Timeout doesn't work as expected in JS SDK #275

Closed mlejva closed 3 months ago

mlejva commented 10 months ago

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

jamesmurdza commented 10 months ago

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.

mlejva commented 10 months ago

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!

jamesmurdza commented 10 months ago

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.

ValentaTomas commented 3 months ago

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