Community
Participate
Working Groups
Created attachment 265697 [details] Line 54, "com.android.browser.BrowserActivity" This is a really weird bug. I attached the source where it happens (on line 54, "com.android.browser.BrowserActivity"). Exception is thrown only in case when I use my code which doesn't modify ast, but just call getLiteralValue() (in wrong moment?), I wasn't able to reproduce it by making compilation unit of the file and calling getLiteralValue() for each StringLiteral (via ASTVisitor). The bug doesn't occur if I use debugger to catch the moment of "throw" to give you more details. I've observed that if I hit breakpoints for all throw new InvalidInputException(smth); in class org.eclipse.jdt.internal.compiler.parser.Scanner because public String getLiteralValue() { String s = getEscapedValue(); int len = s.length(); if (len < 2 || s.charAt(0) != '\"' || s.charAt(len-1) != '\"' ) { throw new IllegalArgumentException(); } Scanner scanner = this.ast.scanner; char[] source = s.toCharArray(); scanner.setSource(source); scanner.resetTo(0, source.length); try { int tokenType = scanner.getNextToken(); // exception is thrown somewhere here exception is not thrown and code finishes fine. But if breakpoints are missing, it always throws IllegalArgumentException
Probably the source is looking like fine, and the details are not enough. Please tell me what else I can provide
This is typically the type of problem that cannot be solved without being able to debug the problem directly. So unless you can provide a reproducable test case, I doubt there is much we can do to help.
Hi. I figured out, it can be reproduced pretty easy Just call in multiple threads sl.getLiteralValue(); // StringLiteral sl for any string literal Let me know if you were not able to reproduce this, and I will attach my sources I receive the exception sent previously or this one: java.lang.StringIndexOutOfBoundsException: String index out of range: -1 at java.lang.String.<init>(Unknown Source) at org.eclipse.jdt.internal.compiler.parser.Scanner.getCurrentStringLiteral(Scanner.java:534) at org.eclipse.jdt.core.dom.StringLiteral.getLiteralValue(StringLiteral.java:238)
Hi, please let me know that you read my message. There's a poc public static void main(String[] args) { ASTParser astParser = ASTParser.newParser(AST.JLS8); astParser.setKind(ASTParser.K_COMPILATION_UNIT); astParser.setSource("class Test { public String x = \"x\"; }".toCharArray()); ASTNode astNode = astParser.createAST(null); AtomicReference<StringLiteral> ref = new AtomicReference<StringLiteral>(); astNode.accept(new ASTVisitor() { public boolean visit(StringLiteral st) { ref.set(st); return false; } }); StringLiteral stringLiteral = ref.get(); for(int i = 1; i <= 10; i++) { Thread thread = new Thread(new Runnable() { public void run() { while(true) { stringLiteral.getLiteralValue(); } } }); thread.start(); } }
I am not sure the AST is supposed to be used in a multithread environment. You might want to handle concurrent calls to the nodes. Jay, any thoughts?
Hi. Any changes? Will it be fixed? I've noticed a lot of synchronized wrappers, so I assume that multiple threads should be supported
(In reply to Olivier Thomann from comment #5) > I am not sure the AST is supposed to be used in a multithread environment. > You might want to handle concurrent calls to the nodes. > Jay, any thoughts? Sorry, I missed this. Copying Manoj, who is the right person to answer this.
Hi, any changes? Doesn't seem to be fixed, but last comment was long time ago
(In reply to Jay Arthanareeswaran from comment #7) > (In reply to Olivier Thomann from comment #5) > > I am not sure the AST is supposed to be used in a multithread environment. > > You might want to handle concurrent calls to the nodes. > > Jay, any thoughts? > > Sorry, I missed this. > > Copying Manoj, who is the right person to answer this. Same here.. missed this. For a starter, AST is not meant to be used in a multi-thread environment. @Sarika: Can you please investigate and take it forward? TIA.
I talked with Stephan Hermann, and he told that some binding APIs are not designed to work like this, but not ASTs. So JDT can resolve string literal values during AST build time, but not each time when called. Can you please also clarify to me, why there are synchronized statements in DefaultBindingResolver class? Or it tries to handle some multitheading calls?
@Manoj, I will need some inputs to proceed. AST is not meant to be used in a multi-thread environment -> Should we restrict the usage then in a must-thread environment?
Bulk move out of 4.14
The javadoc of class ASTNode has this: "AST nodes are thread-safe for readers provided there are no active writers." This promise is violated by the accessor StringLiteral.getLiteralValue() which changes state of the shared scanner, an operation that cannot be thread-safe without further protection. Options: (1) Protect all critical sections using the scanner, e.g., using a protocol of Scanner acquireScanner(); // blocks if already acquired by another thread void releaseScanner(); (2) Introduce a method for cloning a Scanner. Option (1) introduces new risks of deadlock. Option (2) could incur some performance penalty, but if not called in a tight loop it should not be too bad. We should check how much state must actually cloned for the given operations. I believe it is mostly the source reference and compliance settings. At a quick scan I only found one more usage of scanner that changes state in an operation that is supposed to be thread safe: CharLiteral.charValue().
(In reply to Stephan Herrmann from comment #13) > The javadoc of class ASTNode has this: > > "AST nodes are thread-safe for readers provided there are no active writers." > > This promise is violated by the accessor StringLiteral.getLiteralValue() > which changes state of the shared scanner, an operation that cannot be > thread-safe without further protection. > > Options: > > (1) Protect all critical sections using the scanner, e.g., using a protocol > of > Scanner acquireScanner(); // blocks if already acquired by another thread > void releaseScanner(); > > (2) Introduce a method for cloning a Scanner. > > > Option (1) introduces new risks of deadlock. > > Option (2) could incur some performance penalty, but if not called in a > tight loop it should not be too bad. We should check how much state must > actually cloned for the given operations. I believe it is mostly the source > reference and compliance settings. > > At a quick scan I only found one more usage of scanner that changes state in > an operation that is supposed to be thread safe: CharLiteral.charValue(). I agree to cloning as a better option. And we can move the static declarations to a different class to avoid duplication.
Bulk move out of 4.19 M1
Removing the target milestone as we don't have any owner of this bug.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
