Tim Ellison wrote:
Paulex Yang wrote:
Tim,
Good catch! But I'm afraid it not only a test defect, but also a
possible Harmony implementation bug.
The JavaDoc for String says nothing about synchronization, but Charset's
does: " All of the methods defined in this class are safe for use by
multiple concurrent threads.". So it should be safe to invoke
Charset.encode() concurrently.
Yep, so if that had been used then I would have complained differently,
but since String doesn't claim to be thread-safe I moved it out.
Looking inside the Charset.encode(CharBuffer), it has got lock on
itself, the cause is the system wide cached CharsetEncoder instance is
not synchronized[1], so if String.getBytes() always use
Charset.forName() to get the cached charset instance for some certain
encoding, the error won't happen. Probably String create a new Charset
every time.
Agreed, if the String was not using the 'lastCharset' it will create
multiple instances. Not sure why it would have this churn, unless there
was some IO on another thread or something with a different charset.
Even so, I think the Charset implementation should be safe
according to spec, so I suggest to also hold lock on the cached
CharsetEncoder during Charset.encode() so that this method is safe
enough to be used concurrently.
comments?
Yes, all instances of Charset should safely use the cached
encoders/decoders. You may then also be able to reduce the
synchronization of the Charset itself (it would be redundant, but would
not matter if two instances cache the same encoder simultaneously).
+1, the Charset.decode() has same issue, I'll raise a JIRA and create
patch for this.
Regards,
Tim
[1] code fragment from Charset.encode()
synchronized public final ByteBuffer encode(CharBuffer buffer) {
CharsetEncoder e = getCachedCharsetEncoder(canonicalName);
...
try {
// e is not safe for concurrent use
return e.encode(buffer);
} catch (CharacterCodingException ex) {
throw new Error(ex.getMessage(), ex);
}
}
Tim Ellison wrote:
The failing test does this (amongst other things):
final int THREAD_NUM = 20;
Thread[] thread = new Thread[THREAD_NUM];
for (int i = 0; i < THREAD_NUM; i++) {
thread[i] = new Thread() {
public void run() {
try {
sink
.write(ByteBuffer.wrap("bytes"
***** .getBytes(ISO8859_1)));
} catch (IOException e) {
throw new RuntimeException(e);
}
}
};
}
for (int i = 0; i < THREAD_NUM; i++) {
thread[i].start();
}
If String#getBytes(String) is not thread safe then we are doomed ;-)
How about:
@@ -104,14 +104,13 @@
public void test_write_LByteBuffer_mutliThread() throws IOException,
InterruptedException {
final int THREAD_NUM = 20;
+ final byte[] strbytes = "bytes".getBytes(ISO8859_1);
Thread[] thread = new Thread[THREAD_NUM];
for (int i = 0; i < THREAD_NUM; i++) {
thread[i] = new Thread() {
public void run() {
try {
- sink
- .write(ByteBuffer.wrap("bytes"
- .getBytes(ISO8859_1)));
+ sink.write(ByteBuffer.wrap(strbytes));
} catch (IOException e) {
throw new RuntimeException(e);
}
Regards,
Tim
Apache Harmony Build wrote:
<snip>
[exec] [junit] java.lang.IllegalStateException
[exec] [junit] at
java.nio.charset.CharsetEncoder.encode(CharsetEncoder.java:480)
[exec] [junit] at
java.nio.charset.CharsetEncoder.encode(CharsetEncoder.java:347)
[exec] [junit] at
java.nio.charset.Charset.encode(Charset.java:639)
[exec] [junit] at
java.lang.String.getBytes(String.java:838)
[exec] [junit] at
org.apache.harmony.tests.java.nio.channels.SinkChannelTest$1.run(SinkChannelTest.java:112)
[exec] [junit] Tests FAILED
--
Paulex Yang
China Software Development Lab
IBM
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
