Ability to check whether data is available or a read timeout
marciot opened this issue · 23 comments
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()
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.
Hmmm. I'm unsure exactly what problem you're trying to solve here:
-
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.
-
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.
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.
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.
OK, cool. Yeah, so my personal view of the ideal solution here would be:
-
Make read requests easily abortable using an
AbortSignal: whatwg/streams#1103 -
Make it easy to generate
AbortSignals which abort after a timeout: whatwg/dom#951
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;
}
}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()".
OK, cool. Yeah, so my personal view of the ideal solution here would be:
- Make read requests easily abortable using an
AbortSignal: whatwg/streams#1103- Make it easy to generate
AbortSignals which abort after a timeout: whatwg/dom#951Then you would write
getReplyWithTimeout(10_000)astry { 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!
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...
@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.
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.
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.
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.
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.
Purging data from the port can be accomplished by calling
abort()on theReadableStream(or reader). This will return aPromiseand close the stream. Once thePromiseresolves you can access thereadableattribute on theSerialPortto get a newReadableStream.
@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.
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.
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!
No offense taken, this is really good feedback to help us make the API better.
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?
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;
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.
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
TypeErrorif 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!
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.
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. 😄