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

Ability to check whether data is available or a read timeout #122

Closed
marciot opened this issue Feb 24, 2021 · 23 comments · Fixed by #167
Closed

Ability to check whether data is available or a read timeout #122

marciot opened this issue Feb 24, 2021 · 23 comments · Fixed by #167

Comments

@marciot
Copy link

marciot commented Feb 24, 2021

Right now, when you call await serial.readable.getReader().read(), it will block indefinitely if no data is available. This works alright when you know you are expecting data, but it won't work very well if you do not know this for certain or when serial errors cause data to be dropped.

It is possible to implement a read timeout using setTimeout, but I found it is somewhat tricky to get right and I had to go through several iterations to figure out how to make it work:

async read(timeout) {
    let timeoutId;
    let timeoutPromise = new Promise(
            (resolve, reject) =>
                this.timeoutId = setTimeout(
                    () => reject(new SerialTimeout("Timeout expired while waiting for data")),
                    timeout
                )
    );
    if(!this.readPromise) {
        this.readPromise = this.reader.read();
    }
    const result = await Promise.race([this.readPromise, timeoutPromise]);
    this.readPromise = null;
    clearTimeout(this.timeoutId);
    return result;
}

Having a timeout helps in some situations, but it really seems like a clumsy work around to not being able to check whether data is available before calling serial.readable.getReader().read()

@reillyeon
Copy link
Collaborator

This is mostly a feature request for the ReadableStream API so adding @domenic and @ricea for visibility.

In WritableStreamDefaultWriter there is a desiredSize property and I could imagine a similar availableSize attribute being added to ReadableStreamDefaultReader. Note that for the Web Serial API, at least as implemented in Chromium, the desiredSize attribute doesn't work correctly for reasons explained in the specification. We would need similar hooks between the ReadableStream and UnderlyingSource to allow the source to specify how much data it could potentially return from pull() without actually enqueuing it.

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

Hmmm. I'm unsure exactly what problem you're trying to solve here:

  1. Is it reading with a timeout? I can certainly imagine making that more ergonomic in various ways, and if you tell me this is what you're trying to solve, I am happy to go into more detail on potential improvements there.

  2. Is it checking synchronously whether data is available? I'm having a harder time imagining how this would work. Do you poll such a property on a setInterval()? That seems pretty strange, and backward from how we do things in JavaScript typically. I.e. in JS we use actual asynchronous APIs, instead of sync APIs + polling.

@marciot
Copy link
Author

marciot commented Feb 24, 2021

The place where this come into play is 3D printing. For every command you send to a 3D printer, you should expect a reply, this leads to code that looks something like this:

async printLoop(commands) {
   while(commands.length) {
      await sendCommand(commands.pop());
      await getReply();
   }
}

However, for various reasons, such as bugs in the firmware, or slight mismatch in the serial data rates between the computer and the microcontroller used in the printers, occasionally a reply is never received. This leads to a stall in the print, unless the code is modified as such:

async printLoop(commands) {
   while(commands.length) {
      let pendingCommand = commands.shift();
      await sendCommand(pendingCommand);
      try {
         await getReplyWithTimeout(10000);
      } catch(e) {
         if(e instanceof SerialTimeoutError) {
            // Resend the command
            command.unshift(pendingCommand);
            continue;
         } else {
             throw e;
         }
      }
   }
}

As for the second question, I didn't have a specific use case in mind, except to think that it could possibly be used to solve the problem presented in the first situation. It's possible that indeed it isn't necessary, if a timeout facility was provided.

@reillyeon
Copy link
Collaborator

reillyeon commented Feb 24, 2021

To handle timeouts whatwg/streams#1103 is probably what you need. That assumes, however, that you actually want to cancel the call to read(). This isn't strictly necessary. Some piece of the code which is listening for messages can be kept active at all times waiting for it to respond. As written in the original example there's actually a problem because since the read is never cancelled the next chunk, when it arrives, will be discarded.

I'm curious if developer using WebSocketStreams are running into similar issues. Ignoring HTTP streaming, for a fetch-based stream you don't find yourself handling discrete chunks of the data over a long time-scale the way you do for a long-lasting connection.

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

OK, cool. Yeah, so my personal view of the ideal solution here would be:

Then you would write getReplyWithTimeout(10_000) as

try {
  const { value, done } = await serialReader.read({ signal: AbortSignal.timeout(10_000) });
} catch (e) {
  if (e.name === "AbortError") {
    // TODO
  } else {
    throw e;
  }
}

@marciot
Copy link
Author

marciot commented Feb 24, 2021

To handle timeouts whatwg/streams#1103 is probably what you need. That assumes, however, that you actually want to cancel the call to read(). This isn't strictly necessary. Some piece of the code which is listening for messages can be kept active at all times waiting for it to respond. As written in the original example there's actually a problem because since the read is never cancelled the next chunk, when it arrives, will be discarded.

Actually, I believe this criticism applies to my first implementation, which looked like this:

async read(timeout) {
    let timeoutId;
    let timeoutPromise = new Promise(
            (resolve, reject) =>
                this.timeoutId = setTimeout(
                    () => reject(new SerialTimeout("Timeout expired while waiting for data")),
                    timeout
                )
    );
    const result = await Promise.race([this.reader.read(), timeoutPromise]);
    clearTimeout(this.timeoutId);
    return result;
}

This version is actually broken in precisely the way you explained. This is why I said writing the code correctly is non-trivial. I fixed it by saving the promise object for reuse in the next call to "read()".

@marciot
Copy link
Author

marciot commented Feb 24, 2021

OK, cool. Yeah, so my personal view of the ideal solution here would be:

Then you would write getReplyWithTimeout(10_000) as

try {
  const { value, done } = await serialReader.read({ signal: AbortSignal.timeout(10_000) });
} catch (e) {
  if (e.name === "AbortError") {
    // TODO
  } else {
    throw e;
  }
}

@domenic: This would work for me. The syntax is a bit strange, I would have expected const { value, done } = await serialReader.read({timeout:10_000});, but I am not familiar with signals so it's just probably my unfamiliarity with them.

Anyhow, it seems like my request is a duplicate of that other ticket. Glad it's being looked at. Thank you for your feedback!

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

In whatwg/streams#1103 folks are pointing out that it's very rare that you want to cancel a read() call without canceling other subsequent read() calls. At first I thought this thread was a counterexample, but now I am not so sure. In particular, @reillyeon's point

That assumes, however, that you actually want to cancel the call to read(). This isn't strictly necessary. Some piece of the code which is listening for messages can be kept active at all times waiting for it to respond.

indicates a different strategy, which is more like

async printLoop(commands) {
   while(commands.length) {
      let pendingCommand = commands.shift();
      await sendCommand(pendingCommand);

      let gotReply = false;
      setTimeout(() => {
        if (!gotReply) {
          // Resend the command
          sendCommand(pendingCommand);
        }
      }, 10_000);

      await getReply(); // no timeout
      gotReply = true;
   }
}

although that only handles a single retry, hmm...

@marciot
Copy link
Author

marciot commented Feb 24, 2021

@domenic: This is a very simplified example. The actual code for interacting with the printer is much more complicated that this. Of the top of my head, simply resending a command like this would probably not work.

Also, I believe that the idea behind async/await is to allow people to write code with a more familiar synchronous paradigm. Adding a timeout like that breaks that abstraction in a way that concerns me a lot.

@marciot
Copy link
Author

marciot commented Feb 24, 2021

Here is an example that still is simplified, but gets closer to what actually needs to happen:

async printLoop(commands) {
   while(commands.length) {
      let pendingCommand = commands.shift();
      await sendCommand(pendingCommand);
      try {
         await getReplyWithTimeout(10000);
      } catch(e) {
         if(e instanceof SerialTimeoutError) {
            // Resend the command later
            command.unshift(pendingCommand);
            // Put printer back into a known state
            await sendResetCommandToPrinter();
            // Purge the input buffers
            while(serial.dataAvailable) {
                serial.read();
            }
            continue;
         } else {
             throw e;
         }
      }
   }
}

Purging the data on the port is one situation where checking whether bytes are available could help; doing so with a read() with timeout introduces a slight delay, which isn't a problem in this case.

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

The point we're trying to make is that you should be able to completely avoid the read-with-timeout pattern, and instead have a concurrent bit of code running which notices that a response hasn't arrived yet, and performs any necessary resets at that time.

That way, you maintain the invariant that a single read request always results in a single result, instead of having to issue potentially N read requests, the first N - 1 of which timeout.

@reillyeon
Copy link
Collaborator

Purging data from the port can be accomplished by calling abort() on the ReadableStream (or reader). This will return a Promise and close the stream. Once the Promise resolves you can access the readable attribute on the SerialPort to get a new ReadableStream.

@marciot
Copy link
Author

marciot commented Feb 24, 2021

The point we're trying to make is that you should be able to completely avoid the read-with-timeout pattern, and instead have a concurrent bit of code running which notices that a response hasn't arrived yet, and performs any necessary resets at that time.

That way, you maintain the invariant that a single read request always results in a single result, instead of having to issue potentially N read requests, the first N - 1 of which timeout.

Hummm. I see what you are saying. I'm not arguing that it can't be done that way. But I guess I will counter that at least a certain number of people will be porting serial code that was written in a sequential manner. A lot of serial port code is written that way and will rely on the timeout pattern; forcing that code to be rewritten as concurrent code could present significant problems.

@marciot
Copy link
Author

marciot commented Feb 24, 2021

Purging data from the port can be accomplished by calling abort() on the ReadableStream (or reader). This will return a Promise and close the stream. Once the Promise resolves you can access the readable attribute on the SerialPort to get a new ReadableStream.

@reillyeon: That's interesting and somewhat non-obvious. I'll have to give that a try. I guess I'm a bit curious why the Web Serial API is so different from the serial interfaces used in the past. I imagine it will present somewhat of a learning curve for folks transitioning to it from other languages.

@reillyeon
Copy link
Collaborator

This (and to a lesser extent #123) is a consequence of our choice to use the WHATWG Streams API instead of providing a more POSIX-style interface. I think that was the right decision for consistency with the rest of the web platform but acknowledge the learning curve.

@marciot
Copy link
Author

marciot commented Feb 24, 2021

This (and to a lesser extent #123) is a consequence of our choice to use the WHATWG Streams API instead of providing a more POSIX-style interface. I think that was the right decision for consistency with the rest of the web platform but acknowledge the learning curve.

@reillyeon Okay. By no means do not take my comments as criticism. The work you guys are doing is amazingly awesome and I am very excited about what I've been able to do with it already.

I don't know whether you guys have ever done 3D printing, here is what I am currently working on:

https://syndaverco.github.io/slicer-beta/?enableSerial=1

Before Web Serial, I had this as an Electron app because it was the only way to access the native serial ports. I am very excited to see it running on a plain old web broswer now. I think it's quite a game changer!

@reillyeon
Copy link
Collaborator

No offense taken, this is really good feedback to help us make the API better.

@reillyeon
Copy link
Collaborator

reillyeon commented Jan 14, 2022

It looks like whatwg/streams#1103 has reached the conclusion that releaseLock() can be used to cancel read() without aborting the stream. Implementing a timeout is thus:

async function readWithTimeout(stream, timeout) {
  const reader = stream.getReader();
  const timer = setTimeout(() => {
    reader.releaseLock();
  }, timeout);
  const result = await reader.read();
  clearTimeout(timer);
  reader.releaseLock();
  return result;
}

This function will reject with a TypeError if the timeout expires.

@domenic, do you know when this new behavior will be implemented in Chromium?

@amirtu
Copy link

amirtu commented Jul 7, 2022

Right now, when you call await serial.readable.getReader().read(), it will block indefinitely if no data is available. This works alright when you know you are expecting data, but it won't work very well if you do not know this for certain or when serial errors cause data to be dropped.

It is possible to implement a read timeout using setTimeout, but I found it is somewhat tricky to get right and I had to go through several iterations to figure out how to make it work:

async read(timeout) {
    let timeoutId;
    let timeoutPromise = new Promise(
            (resolve, reject) =>
                this.timeoutId = setTimeout(
                    () => reject(new SerialTimeout("Timeout expired while waiting for data")),
                    timeout
                )
    );
    if(!this.readPromise) {
        this.readPromise = this.reader.read();
    }
    const result = await Promise.race([this.readPromise, timeoutPromise]);
    this.readPromise = null;
    clearTimeout(this.timeoutId);
    return result;
}

Having a timeout helps in some situations, but it really seems like a clumsy work around to not being able to check whether data is available before calling serial.readable.getReader().read()

I think it is better to substitute the last four lines of code with something like this:

    return Promise
        .race([readPromise, timeoutPromise])
        .then((result) => {
            readPromise = null;
            return result;
        })
        .finally(() => {
            clearTimeout(timeoutId);
        });

UPD. Unfortunately I misunderstood the read function's code earlier, so please ignore my previous statement.
There is still, however, a minor issue: if reader.read(), as noted here, throws an exception to indicate some non-fatal serial port read error, then we will stuck with the current readPromise value.

I think therefore we better to do something like this:

...
readPromise = reader.read()
            .then(
                ({ value, done }) => {
                    if (done) {
                        return {
                            value: null, error: new Error('Reader has been canceled')
                        };
                    }
                    return {
                        value: value, error: null
                    };
                },
                (error) => {
                    return {
                        value: null, error: error
                    };
                });
...
const { value, error } = await Promise.race([this.readPromise, timeoutPromise]);
this.readPromise = null;
clearTimeout(this.timeoutId);
if (error) {
    throw error;
}
return value;

@reillyeon
Copy link
Collaborator

It looks like the Chromium issue tracking implementing the new behavior for releaseLock() was just marked as started so the code in #122 (comment) should start to work soon. Follow that issue for updates. Once that is resolved I will add a developer note to the specification recommending this method for implementing a read timeout and close this issue.

@maehw
Copy link

maehw commented Feb 13, 2024

Hi all. Sorry, if this is not the appropriate channel to ask questions!

I think I am one of the people having that "learning curve" mentioned above.

I stumbled across the async function readWithTimeout(port, timeout) snippet and I think this issue is where it all began. I'd also like to use it. When I use it, I do not get thrown an exception when I can read a reply to a command I've sent. However, when there's no data received at all, I get the:

Uncaught (in promise) TypeError: Releasing Default reader
    at script.js:96:12

where line 96 is the reader.releaseLock(); call inside setTimeout(...).

According to the post from above, this is expected:

This function will reject with a TypeError if the timeout expires.

Can I add a try() { } catch() { } block to handle this? If so, how and where? I failed at trying to wrap the existing code at different places. This would be big help for me! - And maybe also to others if this was added in the example.

Thanks a lot!

@reillyeon
Copy link
Collaborator

reillyeon commented Feb 13, 2024

Can I add a try() { } catch() { } block to handle this? If so, how and where?

You want to put it around any place where you call readWithTimeout() to handle the timeout. E.g.

try {
  const { value, done } = await readWithTimeout(port.readable, 1000);
  // Do something with |value|.
} catch (e) {
  if (e instanceof TypeError) {
    // Handle the timeout.
  } else {
    // Handle other errors.
  }
}

Alternately you can put the try/catch block around the read() inside readWithTimeout() and have the function return some other value on timeout.

@maehw
Copy link

maehw commented Feb 13, 2024

Thank you for your help. I am kind of new to JavaScript, especially the asynchronous part (not really familiar with await, async and Promise).

The following does the trick for me and hence, I am sharing it for others:

async function readWithTimeout(port, timeout) {
  const timer = setTimeout(() => {
    reader.releaseLock();
  }, timeout);
  let result = {done: false, value: new Uint8Array()};
  try {
    result = await reader.read();
  }
  catch (e) {
    if (e instanceof TypeError) {
      console.log("Timeout error occurred!");
    }
    else {
      throw(e);
    }
  }
  clearTimeout(timer);
  //reader.releaseLock();
  return result.value;
}

This should return always return an Uint8Array(). Sometimes an empty one (timeout occurred) and otherwise some data (Uint8Array(1..N)). Not sure if am following good practice here, but works for me™. Thanks for your support and making it possible for me to jump into the Web Serial API world!


Edit: does not work fully for me. Chaining multiple await readWithTimeout() calls (with some time in between) will always run into a timeout for the second call and hence return my an empty array - where I am sure that the serial device receives some data. Is it okay to ask follow-up questions here or is there a more appropriate channel? Or should I open another issue?


Another edit: Commenting out the second reader.releaseLock(); call in the non-timeout case seems to do the trick. For now. Let's see. 😄

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

Successfully merging a pull request may close this issue.

5 participants