Do we even need to limit to 128 chars here? Sems lie decoderInRaw has no fixed buffer size, so maybe you can just push all of buf in there? If this has performance issues (e.g. buf is a lot bigger than 128 so memory usages increases or something), then maybe the incomingbuf should be limited?

It should be fairly easy to just read from the serial port in 128-byte blocks, moving the loop here:

(the argument togetBytes()is the number of bytes to read.

The PipedInputStream has a fixed circular buffer size, if exceeded thedecoderInRaw.write(...) call will block forever (unless we spawn a thread that reads concurrently fromdecoderOutputUTF8)... but I would like to avoid complicating it again.

We can increase the buffer size to something bigger like 16Kb to reduce the function call pressure, I'll push a change for this.

Ah, I see. I can't find the default size anywhere, though, so hardcoding 128 might not be ideal. Passing an explicit buffer size would be better (probably through aBUFFER_SIZE constant or something). The buffer size shouldprobably be a few bytes bigger than the max write size, in case some bytes are left in the buffer. A unittest for this would be to write a partial utf-8 character and then a full buffer worth of bytes.

but I would like to avoid complicating it again.

Wouldn't moving the loop only slightly simplify things? It could result in more function calls andchars allocations maybe, but the latter could easily be an instance variable if this is an issue.

The buffer size should probably be a few bytes bigger than the max write size, in case some bytes are left in the buffer. A unittest for this would be to write a partial utf-8 character and then a full buffer worth of bytes.

yep look my last commit.

Wouldn't moving the loop only slightly simplify things? It could result in more function calls and chars allocations maybe, but the latter could easily be an instance variable if this is an issue.

I don't know, adding a thread would already increase the number of LOC, also you need to figure out how to synchronize it. I'm not going into that path... :-) if you want to give it a try I may wait for it before merging this one.

The main problem is that JSSC doesn't implement theInputStream interface, otherwise we could have chained it to the InputStreamReader directly, without the need of this buffering. Unfortunately it provides this weirdgetBytes() method, so we cannot avoid this boilerplate...

I don't know, adding a thread would already increase the number of LOC, also you need to figure out how to synchronize it. I'm not going into that path... :-) if you want to give it a try I may wait for it before merging this one.

I wasn't suggesting adding a thread, just letserialEvent loop and read BUFFER_SIZE bytes at a time (untilserialEvent.getEventValue() have been read) and callingprocessSerialEvent for each buffer just as now. This would be very similar, except that it ensures that thebuf variable is also limited toBUFFER_SIZE.

The buffer size should probably be a few bytes bigger than the max write size, in case some bytes are left in the buffer.

I thought about this again, and I think this is not actually needed. TheInputStreamReader does the decoding and the buffer is in thePipedInputStream. Looking at theInputStream interface, there is no way to peek at bytes or re-insert them after reading them, so the only way forInputStreamReader to decide there are no additional characters is to read all of them, so I suppose there is a tiny buffer inInputStreamReader for partial characters?

Docs forInputStreamReader also suggest that there is some internal buffering:

Each invocation of one of an InputStreamReader's read() methods may cause one or more bytes to be read from the underlying byte-input stream. To enable the efficient conversion of bytes to characters, more bytes may be read ahead from the underlying stream than are necessary to satisfy the current read operation.

Loooking atdocs forReader.read(char[]), so also InputStreamReader, I see:

Reads characters into an array. This method will block until some input is available, an I/O error occurs, or the end of the stream is reached.

I wonder what this means exactly. Two risks:

1. If no full character is available (e.g.processSerialEvent was called with a partial utf-8 character, or 16kbyte + a partial utf-8 character), this might block and deadlock?
2. Does this guarantee that it reads as much as possible from theInputStream (untilchars is full)? If it could read less, leaving some bytes in the pipe, you'd also get a deadlock.

Problem 1 does indeed seem to occur. This program deadlocks for me:

import java.io.*;import java.nio.charset.*;class Foo {        public static void main(String[] args) throws IOException {                String str = "\u2e2e";                byte[] bytes = str.getBytes(StandardCharsets.UTF_8);                System.out.println(bytes.length);                PipedOutputStream pos = new PipedOutputStream();                PipedInputStream pis = new PipedInputStream(pos);                InputStreamReader isr = new InputStreamReader(pis);                pos.write(bytes, 0, bytes.length - 1);                System.out.println(isr.ready());                int c = isr.read();                System.out.println(String.format("0x%04X", c));/* Originally I tested read(chars), which also blocks.                char chars[] = new char[10];                int n = isr.read(chars);                System.out.println(n);                System.out.println(String.format("0x%04X", Character.codePointAt(chars, 0)));*/        }}

The docs suggest thatready()` can be used to check this:

True if the next read() is guaranteed not to block for input, false otherwise. Note that returning false does not guarantee that the next read will block.

But looking at theStreamDecoder.inReady() source, which is used byInputStreamReader.ready(), this actually returns true when theInputStream hasany bytes available, rather than actually pre-reading bytes to see if there is anything actually available. And indeed,ready() returns true, and then reading blocks...

InputStreamReader.ready() does specify what it does correctly (return true when bytes are available), but then still (incorrectly) promises it won't block.

So I wonder ifInputStreamReader is useful at all if we cannot guarantee it never blocks (and stop calling read()` when there's < 4 bytes in the buffer would prevent blocking, but also prevent the last few bytes before a pause from being read...).

The main problem is that JSSC doesn't implement the InputStream interface

Yeah, indeed. I wonder: Wouldn't a simple wrapper be possible here to allow removing the pipe stuff and simplifying the chain? I think this could remove one buffer from the chain. With this PR, we have:

• SerialPort.readBytes allocates a buffer, which we store inbuf.
• This gets written into the pipe, which as an internal buffer
• This gets read byInputStreamReader, into its internal buffer
• After decoding, this is written to thechars buffer.

With a wrapper, you could do:

• SerialPort.readBytes allocates a buffer, which we store in an internal buffer in the wrapper.
• When the wrapper'sread() method is called byInputStreamReader (passing, ISR's internal buffer array), data is copied to ISR's buffer.
• After decoding, this is written to thechars buffer.

Note that the current code also uses 4 buffers (buf, aByteBuffer aCharBuffer andchars, but the latter could be removed, see below).

I had a bit of a look at how this would look, but given the deadlock problem above must be solved first, I stopped doing so :-)

Seems we also can't fix this by making a non-blockingInputStream to wrapInputStreamReader, since that triggers an exception:http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/nio/cs/StreamDecoder.java#l288

Looking at the core implementation ofInputStreamReader (which defers toStreamDecoder.implRead(), it seems they essentially do the same trick as our original code (Write to aByteBuffer, feed that into aCharsetDecoder, which puts the result in aCharBuffer).

Looking at this PR now, it is indeed simpler code, but adds one buffer in the chain, adds several layers of extra objects (pipes, InputStreamReader, StreamDecoder) and on top of that, seems to deadlock.

So, maybe the original approach should be kept, maybe it can be a bit more simplified by reading smaller chunks from the SerialPort? Also, looking at the code again, I think thechars array buffer could be removed as well, by just usingCharBuffer.array() to access that internal buffer directly.

1. If no full character is available (e.g. processSerialEvent was called with a partial utf-8 character, or 16kbyte + a partial utf-8 character), this might block and deadlock?

...and this is a problem indeed, if there is no easy solution I guess we must drop this PR.

So, maybe the original approach should be kept, maybe it can be a bit more simplified by reading smaller chunks from the SerialPort? Also, looking at the code again, I think the chars array buffer could be removed as well, by just using CharBuffer.array() to access that internal buffer directly.

Yes, but besides that, my concern was the error in#8903 that I've got also on my Windows VM after upgrading to the JDK 8u242. Using anInputStreamReader seemed like the best approach, but I guess we need to find another way.

Hm, interesting, especially since theInputStreamReader seems to useByteBuffer internally too. Any idea what changed about thisflip() to cause this? I can't find anything relevant in the docs, e.g. these all look the same:

• https://docs.oracle.com/javase/7/docs/api/java/nio/Buffer.html#flip()
• https://docs.oracle.com/javase/8/docs/api/java/nio/Buffer.html#flip--
• https://docs.oracle.com/javase/10/docs/api/java/nio/Buffer.html#flip()