Here is the unobfuscated stacktrace:
5/31 22:53:56.558 ExtractDatabase.getDatabaseList java.lang.NullPointerException at com.sybase.jdbc4.timedio.InStreamMgr.moreData(InStreamMgr.java:383) at com.sybase.jdbc4.timedio.Dbio.doRead(Dbio.java:373) at com.sybase.jdbc4.timedio.InStreamMgr.readIfOwner(InStreamMgr.java:587) at com.sybase.jdbc4.timedio.InStreamMgr.doRead(InStreamMgr.java:319) at com.sybase.jdbc4.tds.TdsProtocolContext.getChunk(TdsProtocolContext.java:622) at com.sybase.jdbc4.tds.PduInputFormatter.readPacket(PduInputFormatter.java:239) at com.sybase.jdbc4.tds.PduInputFormatter.read(PduInputFormatter.java:72) at com.sybase.jdbc4.tds.TdsInputStream.read(TdsInputStream.java:91) at com.sybase.jdbc4.tds.TdsInputStream.readUnsignedByte(TdsInputStream.java:124) at com.sybase.jdbc4.tds.Tds.nextResult(Tds.java:2934) at com.sybase.jdbc4.jdbc.ResultGetter.nextResult(ResultGetter.java:78) at com.sybase.jdbc4.jdbc.SybStatement.nextResult(SybStatement.java:302) at com.sybase.jdbc4.jdbc.SybStatement.nextResult(SybStatement.java:284) at com.sybase.jdbc4.jdbc.SybStatement.queryLoop(SybStatement.java:2655) at com.sybase.jdbc4.jdbc.SybStatement.executeQuery(SybStatement.java:2641) at com.sybase.jdbc4.jdbc.SybPreparedStatement.executeQuery(SybPreparedStatement.java:274) at com.sybase.jdbc4.jdbc.SybConnection.isClosed(SybConnection.java:2432) at com.aquafold.aquacore.open.rdbms.core.scripts.WrapperConnection.isClosed(WrapperConnection.java:129) at com.aquafold.aquacore.open.rdbms.core.scripts.ConnectionWrapper.establishConnection(ConnectionWrapper.java:57) at com.aquafold.aquacore.open.rdbms.core.scripts.ConnectionWrapper.createStatement(ConnectionWrapper.java:74) at com.aquafold.aquacore.open.rdbms.core.schema.extract.ExtractDatabase.getDatabaseList(ExtractDatabase.java:940) at com.aquafold.aquacore.open.rdbms.core.schema.extract.ExtractDatabase.getDatabases(ExtractDatabase.java:67) at com.aquafold.datastudio.schematree.sybase.SybaseDatabasesNode.getDetailViewTableModel(SybaseDatabasesNode.java:55) at com.common.ui.tree.CTreeNode$1.query(CTreeNode.java:280) at com.aquafold.datastudio.mainframe.DataStudioFrame$24.process(DataStudioFrame.java:931) at com.common.ui.util.BackgroundThread.run(BackgroundThread.java:102)
The NPE is coming from SybConnection.isClosed()
api. And happens only when Detail is enabled since we are trying to retieve the database list to be displayed on the Detail view.
Looks like a timing issue. We should synchronize access to CDetailView api.
Looks like a timing issue. We should synchronize access to CDetailView api.
@asif: pls ask @kin-hong to do a code review once you have a fix.
@asif: pls ask @kin-hong to do a code review once you have a fix.
Root cause:
In ADS, we have 3 trees which shows its content in Detail Panel. Based on Jide Docking framework, we had to add Listener to each Docking Frame instance to handle state changes and show its corresponding detail. Note that Detail Panel content is retrieved asynchronously in a background thread. When we migrated to Intellij ToolWindow framework, we followed the same approach but Intellij does not have state change listener per ToolWindow instead it has for the entire ToolWindowManager. When listener is added for each PlatWindow, we are adding listener to the same ToolWindowManager.
-- PlatToolWindow
public void addDockableFrameListener(ToolWindowManagerListener listener) { AQAWTUtils.waitForEDT(new Runnable() { @Override public void run() { myToolWindowManager.addDockableFrameListener(listener); } }); }
That means we are adding 3 listeners for handling the state changes. And each state change creates a background thread to update the Detail Panel. And this creates concurrency issues.
Root cause:
In ADS, we have 3 trees which shows its content in Detail Panel. Based on Jide Docking framework, we had to add Listener to each Docking Frame instance to handle state changes and show its corresponding detail. Note that Detail Panel content is retrieved asynchronously in a background thread. When we migrated to Intellij ToolWindow framework, we followed the same approach but Intellij does not have state change listener per ToolWindow instead it has for the entire ToolWindowManager. When listener is added for each PlatWindow, we are adding listener to the same ToolWindowManager.
-- PlatToolWindow
public void addDockableFrameListener(ToolWindowManagerListener listener) { AQAWTUtils.waitForEDT(new Runnable() { @Override public void run() { myToolWindowManager.addDockableFrameListener(listener); } }); }
That means we are adding 3 listeners for handling the state changes. And each state change creates a background thread to update the Detail Panel. And this creates concurrency issues.
@Kin-Hong, please review changes as part of SVN r54598.
@Kin-Hong, please review changes as part of SVN r54598.
@asif - with reference to this comment: In r54553 synchronize access was added CDetailView api, but in r54598 synchronize access was removed CDetailView api. So is this comment still correct/relevant (could we remove/update it)?
As far as I can tell, the r54598 consists of two parts. The first part is to remove the spawning of multiple threads in ToolWindowManagerListener#stateChanged thereby reducing the number of concurrent CDetailView#query calls. I have reviewed this part, it looked good.
The second part is to cancel currently running CDetailView#query threads in DataStudioFrame#showDetail. Here is what I know/observe:
1. Calling Thread#interrupt won't actually stop the thread from execution. We notice that each spawned thread still executes the BackgroundThread#success method. The occurrences of SQLException are not reduced by BackgroundThread#cancel calls
2. There are other test scenarios where multiple DataStudioFrame#showDetail calls are still being invoked which are not from ToolWindowManagerListener#stateChanged. For example, expand Sybase ASE server server node to > Databases > DB_LEFT > User Tables and use the up/down arrow key to move between the DB_LEFT and User Tables nodes rapidly - we notice many SQLExceptions are still being generated (most likely due to concurrency)
3. The SQLExceptions are caught and logged (ERROR level) by ExtractStorageSybaseASE#getObjectsByTablespace, so we can't remove these log entries unless we modify the lower level extraction code
Given the above, I am not seeing the need to implement part 2. I would recommend not changing DataStudioFrame#showDetail to reduce chances of regression.
@asif - with reference to this comment: In r54553 synchronize access was added CDetailView api, but in r54598 synchronize access was removed CDetailView api. So is this comment still correct/relevant (could we remove/update it)?
As far as I can tell, the r54598 consists of two parts. The first part is to remove the spawning of multiple threads in ToolWindowManagerListener#stateChanged thereby reducing the number of concurrent CDetailView#query calls. I have reviewed this part, it looked good.
The second part is to cancel currently running CDetailView#query threads in DataStudioFrame#showDetail. Here is what I know/observe:
1. Calling Thread#interrupt won't actually stop the thread from execution. We notice that each spawned thread still executes the BackgroundThread#success method. The occurrences of SQLException are not reduced by BackgroundThread#cancel calls
2. There are other test scenarios where multiple DataStudioFrame#showDetail calls are still being invoked which are not from ToolWindowManagerListener#stateChanged. For example, expand Sybase ASE server server node to > Databases > DB_LEFT > User Tables and use the up/down arrow key to move between the DB_LEFT and User Tables nodes rapidly - we notice many SQLExceptions are still being generated (most likely due to concurrency)
3. The SQLExceptions are caught and logged (ERROR level) by ExtractStorageSybaseASE#getObjectsByTablespace, so we can't remove these log entries unless we modify the lower level extraction code
Given the above, I am not seeing the need to implement part 2. I would recommend not changing DataStudioFrame#showDetail to reduce chances of regression.
@asif - with reference to this comment: In r54553 synchronize access was added CDetailView api, but in r54598 synchronize access was removed CDetailView api. So is this comment still correct/relevant (could we remove/update it)?
@asif - with reference to this comment: In r54553 synchronize access was added CDetailView api, but in r54598 synchronize access was removed CDetailView api. So is this comment still correct/relevant (could we remove/update it)?
The second part is to cancel currently running CDetailView#query threads in DataStudioFrame#showDetail. ...
I agree with you on removing the thread > cancel calls. Cancel calls were made as part of some initial trials. We do not need any longer.
The second part is to cancel currently running CDetailView#query threads in DataStudioFrame#showDetail. ...
I agree with you on removing the thread > cancel calls. Cancel calls were made as part of some initial trials. We do not need any longer.
NPE is not observed while Opening Query analyser with detail window enabled.
Thus marking this issue as verified.
Please see attached verified_15283.png
Verified in ADS 19 alpha-48
NPE is not observed while Opening Query analyser with detail window enabled.
Thus marking this issue as verified.
Please see attached verified_15283.png
Verified in ADS 19 alpha-48
Issue #15283 |
Closed |
Fixed |
Resolved |
Completion |
No due date |
Fixed Build ADS 19.0.0-alpha-28 |
No time estimate |
Here is the unobfuscated stacktrace:
The NPE is coming from
SybConnection.isClosed()
api. And happens only when Detail is enabled since we are trying to retieve the database list to be displayed on the Detail view.