Bugzilla – Full Text Bug Listing
| Summary: | Race conditions in ASTProvider.prepareForUsingCache method | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Sergey Prigogin <eclipse.sprigogin> |
| Component: | cdt-editor | Assignee: | Project Inbox <cdt-editor-inbox> |
| Status: | RESOLVED INVALID | QA Contact: | Anton Leherbauer <aleherb+eclipse> |
| Severity: | normal | ||
| Priority: | P3 | CC: | cdtdoug, malaperle |
| Version: | 8.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
I saw you already changed the synchronization. Please make sure that bug 233976 does not reoccur. (In reply to comment #1) > I saw you already changed the synchronization. Please make sure that bug > 233976 does not reoccur. Toni, do you mind reviewing the fix? I'm particularly interested in your assessment of the second race condition, which haven't been addressed by the fix. I can easily create a deadlock: Thread [main] (Suspended) owns: Object (id=89) waiting for: Object (id=90) owned by: Thread [Worker-4] (Suspended) ASTCache.aboutToBeReconciled(ITranslationUnit) line: 386 ASTProvider.aboutToBeReconciled(ICElement) line: 274 CEditor.aboutToBeReconciled() line: 3039 CReconcilingStrategy.aboutToBeReconciled() line: 133 CCompositeReconcilingStrategy.aboutToBeReconciled() line: 121 CReconciler.aboutToBeReconciled() line: 386 AbstractReconciler$Listener.documentChanged(DocumentEvent) line: 241 SynchronizableDocument(AbstractDocument).doFireDocumentChanged2(DocumentEvent) line: 769 SynchronizableDocument(AbstractDocument).doFireDocumentChanged(DocumentEvent, boolean, IRegion) line: 736 SynchronizableDocument(AbstractDocument).doFireDocumentChanged(DocumentEvent) line: 721 SynchronizableDocument(AbstractDocument).fireDocumentChanged(DocumentEvent) line: 796 SynchronizableDocument(AbstractDocument).replace(int, int, String, long) line: 1191 SynchronizableDocument.replace(int, int, String, long) line: 194 SynchronizableDocument(AbstractDocument).replace(int, int, String) line: 1210 SynchronizableDocument.replace(int, int, String) line: 180 DefaultDocumentAdapter.replaceTextRange(int, int, String) line: 248 StyledText.modifyContent(Event, boolean) line: 7178 StyledText.sendKeyEvent(Event) line: 7992 StyledText.doContent(char) line: 2456 StyledText.handleKey(Event) line: 5909 StyledText.handleKeyDown(Event) line: 5939 StyledText$7.handleEvent(Event) line: 5633 EventTable.sendEvent(Event) line: 84 StyledText(Widget).sendEvent(Event) line: 1053 StyledText(Widget).sendEvent(int, Event, boolean) line: 1077 StyledText(Widget).sendEvent(int, Event) line: 1062 StyledText(Widget).sendKeyEvent(int, int, int, int, Event) line: 1104 StyledText(Widget).sendKeyEvent(int, int, int, int) line: 1100 StyledText(Widget).wmChar(int, int, int) line: 1509 StyledText(Control).WM_CHAR(int, int) line: 4623 StyledText(Canvas).WM_CHAR(int, int) line: 345 StyledText(Control).windowProc(int, int, int, int) line: 4511 StyledText(Canvas).windowProc(int, int, int, int) line: 341 Display.windowProc(int, int, int, int) line: 4957 OS.DispatchMessageW(MSG) line: not available [native method] OS.DispatchMessage(MSG) line: 2525 Display.readAndDispatch() line: 3737 Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2696 Workbench.runUI() line: 2660 Workbench.access$4(Workbench) line: 2494 Workbench$7.run() line: 674 Realm.runWithDefault(Realm, Runnable) line: 332 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 667 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 123 EclipseAppHandle.run(Object) line: 196 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 344 EclipseStarter.run(String[], Runnable) line: 179 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: not available DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: not available Method.invoke(Object, Object...) line: not available Main.invokeFramework(String[], URL[]) line: 622 Main.basicRun(String[]) line: 577 Main.run(String[]) line: 1410 Main.main(String[]) line: 1386 Thread [Worker-4] (Suspended) owns: Object (id=90) waited by: Thread [Worker-5] (Running) waited by: Thread [main] (Suspended) Unsafe.park(boolean, long) line: not available [native method] LockSupport.park(Object) line: not available Semaphore$NonfairSync(AbstractQueuedSynchronizer).parkAndCheckInterrupt() line: not available Semaphore$NonfairSync(AbstractQueuedSynchronizer).doAcquireSharedInterruptibly(int) line: not available Semaphore$NonfairSync(AbstractQueuedSynchronizer).acquireSharedInterruptibly(int) line: not available Semaphore.acquire() line: not available ASTCache.acquireSharedAST(ITranslationUnit, IIndex, boolean, IProgressMonitor) line: 248 ASTCache.runOnAST(ITranslationUnit, boolean, IProgressMonitor, ASTCache$ASTRunnable) line: 223 ASTProvider.runOnAST(ICElement, ASTProvider$WAIT_FLAG, IProgressMonitor, ASTCache$ASTRunnable) line: 334 SelectionListenerWithASTManager$PartListenerGroup.calculateASTandInform(IWorkingCopy, ITextSelection, IProgressMonitor) line: 168 SelectionListenerWithASTManager$PartListenerGroup$3.run(IProgressMonitor) line: 142 Worker.run() line: 54 I don't understand yet where you see two race conditions and esp. what the real negative impact is. (In reply to comment #3) The deadlock that you reported was introduced by my other changes and not by synchronization around the following block of code that is now located inside prepareForUsingCache method. final boolean isActive= fCache.isActiveElement(tu); if (waitFlag == WAIT_ACTIVE_ONLY && !isActive) { return false; } if (isActive && updateModificationStamp()) { fCache.disposeAST(); } Nevertheless, I removed the synchronization around this block since I realized how little I understand this code. The potential race scenarios are: 1. tu was inactive when fCache.isActiveElement(tu) was called but became active immediately after that. 2. tu was active when fCache.isActiveElement(tu) was called but became inactive immediately after that. 3. updateModificationStamp() returned false, but the document changed immediately after that. #1 is benign since we simply say that the shared AST is not available instead of trying harder to get it. I'm not sure what would happen in scenarios #2 and #3. (In reply to comment #4) > (In reply to comment #3) > The deadlock that you reported was introduced by my other changes and not by > synchronization around the following block of code that is now located inside > prepareForUsingCache method. Right, I did not look in detail at the stack trace, but the synchronization of the method prepreForUsingCache() introduces a potential deadlock as well: Thread [main] (Suspended) owns: Object (id=121) waited by: Thread [Worker-16] (Suspended) owns: ASTProvider (id=126) waited by: Thread [main] (Suspended) waiting for: ASTProvider (id=126) owned by: Thread [Worker-16] (Suspended) waiting for: Object (id=121) ASTProvider.updateModificationStamp() line: 284 ASTProvider.aboutToBeReconciled(ICElement) line: 277 CEditor.aboutToBeReconciled() line: 3039 CReconcilingStrategy.aboutToBeReconciled() line: 133 CCompositeReconcilingStrategy.aboutToBeReconciled() line: 121 CReconciler.aboutToBeReconciled() line: 386 AbstractReconciler$Listener.documentChanged(DocumentEvent) line: 241 Thread [Worker-16] (Suspended) owns: ASTProvider (id=126) waiting for: Object (id=121) owned by: Thread [main] (Suspended) SynchronizableDocument.getModificationStamp() line: 166 ASTProvider.updateModificationStamp() line: 294 ASTProvider.prepareForUsingCache(ITranslationUnit, ASTProvider$WAIT_FLAG) line: 401 ASTProvider.runOnAST(ICElement, ASTProvider$WAIT_FLAG, IProgressMonitor, ASTCache$ASTRunnable) line: 345 SelectionListenerWithASTManager$PartListenerGroup.calculateASTandInform(IWorkingCopy, ITextSelection, IProgressMonitor) line: 168 SelectionListenerWithASTManager$PartListenerGroup$3.run(IProgressMonitor) line: 142 Worker.run() line: 54 > Nevertheless, I removed the synchronization around this block since I realized > how little I understand this code. The potential race scenarios are: > > 1. tu was inactive when fCache.isActiveElement(tu) was called but became active > immediately after that. > 2. tu was active when fCache.isActiveElement(tu) was called but became inactive > immediately after that. > 3. updateModificationStamp() returned false, but the document changed > immediately after that. > #1 and #2 have no impact from my POV. That's just bad luck, but completely valid. Otherwise we would need to block the UI thread changing the active editor during the whole of runOnAST. #3 is no problem as well. updateModificationStamp() is there to protect against using a stale AST if in scalability mode when no reconciler runs. Of course it does not protect against document changes while the AST is in use, but for that we would need to block the UI thread for the whole time. We don't want that, right? This was a red herring. *** cdt cvs genie on behalf of sprigogin *** Bug 342506. Added a comment with a link. [*] ASTProvider.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ASTProvider.java?root=Tools_Project&r1=1.21&r2=1.22 |
The method is defined as: 1 public IStatus runOnAST(ICElement cElement, WAIT_FLAG waitFlag, 2 IProgressMonitor monitor, ASTCache.ASTRunnable astRunnable) { 3 Assert.isTrue(cElement instanceof ITranslationUnit); 4 final ITranslationUnit tu = (ITranslationUnit) cElement; 5 if (!tu.isOpen()) 6 return Status.CANCEL_STATUS; 7 8 final boolean isActive= fCache.isActiveElement(tu); 9 if (waitFlag == WAIT_ACTIVE_ONLY && !isActive) { 10 return Status.CANCEL_STATUS; 11 } 12 if (isActive && updateModificationStamp()) { 13 fCache.disposeAST(); 14 } 15 return fCache.runOnAST(tu, waitFlag != WAIT_NO, monitor, astRunnable); 16} There seems to be a race condition around isActive, which may no longer be valid as soon as fCache.isActiveElement(tu) returns. Another race condition is around the value returned by the updateModificationStamp() call. fCache.disposeAST() may not be called if activeEditorChanged is called between updateModificationStamp() and fCache.runOnAST. It looks like lines 8-14 have to be put inside a synchronized(this) block, but it still does not prevent the second race condition.