From 3b7f5abe7bbbb5e532a267bcf3e6b489abcf4841 Mon Sep 17 00:00:00 2001 From: Alexandre Montplaisir Date: Mon, 5 Nov 2012 11:54:12 -0500 Subject: [PATCH] tmf: Handle race between state system queries and diposal It's possible that a state system gets disposed while a query is ongoing (happens between two successive .getNextChild() calls, for example). Instead of implementing a convoluted reader-writer-lock, we can just catch the ClosedChannelException and rethrow a StateSystemDisposed exception. Change-Id: I93f72860bbb94a63e86e7c6f51682af659599fe3 Signed-off-by: Alexandre Montplaisir Reviewed-on: https://git.eclipse.org/r/8520 Tested-by: Hudson CI Reviewed-by: Patrick Tasse IP-Clean: Patrick Tasse --- .../statesystem/IStateHistoryBackend.java | 16 +- .../core/statesystem/historytree/HT_IO.java | 15 +- .../statesystem/historytree/HistoryTree.java | 143 ++++++++++-------- .../historytree/HistoryTreeBackend.java | 38 +++-- 4 files changed, 130 insertions(+), 82 deletions(-) diff --git a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/IStateHistoryBackend.java b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/IStateHistoryBackend.java index 13c512b9c7..4a44e154b7 100644 --- a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/IStateHistoryBackend.java +++ b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/IStateHistoryBackend.java @@ -18,6 +18,7 @@ import java.io.PrintWriter; import java.util.List; import org.eclipse.linuxtools.tmf.core.exceptions.AttributeNotFoundException; +import org.eclipse.linuxtools.tmf.core.exceptions.StateSystemDisposedException; import org.eclipse.linuxtools.tmf.core.exceptions.TimeRangeException; import org.eclipse.linuxtools.tmf.core.interval.ITmfStateInterval; import org.eclipse.linuxtools.tmf.core.statevalue.ITmfStateValue; @@ -131,9 +132,9 @@ public interface IStateHistoryBackend { */ public void dispose(); - /** - * @name Query methods - */ + // ------------------------------------------------------------------------ + // Query methods + // ------------------------------------------------------------------------ /** * Complete "give me the state at a given time" method 'currentStateInfo' is @@ -147,9 +148,11 @@ public interface IStateHistoryBackend { * Target timestamp of the query * @throws TimeRangeException * If the timestamp is outside of the history/trace + * @throws StateSystemDisposedException + * If the state system is disposed while a request is ongoing. */ public void doQuery(List currentStateInfo, long t) - throws TimeRangeException; + throws TimeRangeException, StateSystemDisposedException; /** * Some providers might want to specify a different way to obtain just a @@ -166,9 +169,12 @@ public interface IStateHistoryBackend { * If the timestamp was invalid * @throws AttributeNotFoundException * If the quark was invalid + * @throws StateSystemDisposedException + * If the state system is disposed while a request is ongoing. */ public ITmfStateInterval doSingularQuery(long t, int attributeQuark) - throws TimeRangeException, AttributeNotFoundException; + throws TimeRangeException, AttributeNotFoundException, + StateSystemDisposedException; /** * Simple check to make sure the requested timestamps are within the borders diff --git a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HT_IO.java b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HT_IO.java index 6eb1ed5bc3..7495eb1b2f 100644 --- a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HT_IO.java +++ b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HT_IO.java @@ -16,6 +16,7 @@ import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.channels.ClosedChannelException; import java.nio.channels.FileChannel; /** @@ -84,8 +85,10 @@ class HT_IO { * @param seqNumber * Sequence number of the node we want * @return The wanted node in object form + * @throws ClosedChannelException + * If the channel was closed before we could read */ - HTNode readNode(int seqNumber) { + HTNode readNode(int seqNumber) throws ClosedChannelException { HTNode node = readNodeFromMemory(seqNumber); if (node == null) { return readNodeFromDisk(seqNumber); @@ -106,14 +109,22 @@ class HT_IO { * This method here isn't private, if we know for sure the node cannot be in * memory it's a bit faster to use this directly (when opening a file from * disk for example) + * + * @throws ClosedChannelException + * Usually happens because the file was closed while we were + * reading. Instead of using a big reader-writer lock, we'll + * just catch this exception. */ - synchronized HTNode readNodeFromDisk(int seqNumber) { + synchronized HTNode readNodeFromDisk(int seqNumber) throws ClosedChannelException { HTNode readNode; try { seekFCToNodePos(fcIn, seqNumber); readNode = HTNode.readNode(tree, fcIn); return readNode; + } catch (ClosedChannelException e) { + throw e; } catch (IOException e) { + /* Other types of IOExceptions shouldn't happen at this point though */ e.printStackTrace(); return null; } diff --git a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTree.java b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTree.java index 05b4d50c8f..576b0490b7 100644 --- a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTree.java +++ b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTree.java @@ -2,12 +2,12 @@ * Copyright (c) 2012 Ericsson * Copyright (c) 2010, 2011 École Polytechnique de Montréal * Copyright (c) 2010, 2011 Alexandre Montplaisir - * + * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License v1.0 which * accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html - * + * *******************************************************************************/ package org.eclipse.linuxtools.internal.tmf.core.statesystem.historytree; @@ -18,6 +18,7 @@ import java.io.IOException; import java.io.PrintWriter; import java.nio.ByteBuffer; import java.nio.ByteOrder; +import java.nio.channels.ClosedChannelException; import java.nio.channels.FileChannel; import java.util.Vector; @@ -26,9 +27,9 @@ import org.eclipse.linuxtools.tmf.core.exceptions.TimeRangeException; /** * Meta-container for the History Tree. This structure contains all the * high-level data relevant to the tree. - * + * * @author alexmont - * + * */ class HistoryTree { @@ -65,7 +66,7 @@ class HistoryTree { /** * Create a new State History from scratch, using a SHTConfig object for * configuration - * + * * @param conf * @throws IOException */ @@ -103,7 +104,7 @@ class HistoryTree { /** * "Reader" constructor : instantiate a SHTree from an existing tree file on * disk - * + * * @param existingFileName * Path/filename of the history-file we are to open * @throws IOException @@ -194,7 +195,7 @@ class HistoryTree { * commit all nodes to disk and then return the RandomAccessFile descriptor * so the Tree object can save its configuration into the header of the * file. - * + * * @param requestedEndTime */ void closeTree(long requestedEndTime) { @@ -202,11 +203,11 @@ class HistoryTree { ByteBuffer buffer; int i, res; - /* + /* * Work-around the "empty branches" that get created when the root node * becomes full. Overwrite the tree's end time with the original wanted * end-time, to ensure no queries are sent into those empty nodes. - * + * * This won't be needed once extended nodes are implemented. */ this.treeEnd = requestedEndTime; @@ -287,12 +288,13 @@ class HistoryTree { * Rebuild the latestBranch "cache" object by reading the nodes from disk * (When we are opening an existing file on disk and want to append to it, * for example). - * + * * @param rootNodeSeqNb * The sequence number of the root node, so we know where to * start + * @throws ClosedChannelException */ - private void rebuildLatestBranch(int rootNodeSeqNb) { + private void rebuildLatestBranch(int rootNodeSeqNb) throws ClosedChannelException { HTNode nextChildNode; this.latestBranch = new Vector(); @@ -307,7 +309,7 @@ class HistoryTree { /** * Insert an interval in the tree - * + * * @param interval */ void insertInterval(HTInterval interval) throws TimeRangeException { @@ -319,7 +321,7 @@ class HistoryTree { /** * Inner method to find in which node we should add the interval. - * + * * @param interval * The interval to add to the tree * @param indexOfNode @@ -364,7 +366,7 @@ class HistoryTree { /** * Method to add a sibling to any node in the latest branch. This will add * children back down to the leaf level, if needed. - * + * * @param indexOfNode * The index in latestBranch where we start adding */ @@ -442,7 +444,7 @@ class HistoryTree { /** * Add a new empty node to the tree. - * + * * @param parentSeqNumber * Sequence number of this node's parent * @param startTime @@ -465,12 +467,14 @@ class HistoryTree { * Inner method to select the next child of the current node intersecting * the given timestamp. Useful for moving down the tree following one * branch. - * + * * @param currentNode * @param t * @return The child node intersecting t + * @throws ClosedChannelException + * If the file channel was closed while we were reading the tree */ - HTNode selectNextChild(CoreNode currentNode, long t) { + HTNode selectNextChild(CoreNode currentNode, long t) throws ClosedChannelException { assert (currentNode.getNbChildren() > 0); int potentialNextSeqNb = currentNode.getSequenceNumber(); @@ -511,9 +515,9 @@ class HistoryTree { return config.stateFile.length(); } - /** - * @name Test/debugging functions - */ + // ------------------------------------------------------------------------ + // Test/debugging methods + // ------------------------------------------------------------------------ /* Only used for debugging, shouldn't be externalized */ @SuppressWarnings("nls") @@ -531,45 +535,50 @@ class HistoryTree { node = (CoreNode) zenode; - /* - * Test that this node's start and end times match the start of the - * first child and the end of the last child, respectively - */ - if (node.getNbChildren() > 0) { - otherNode = treeIO.readNode(node.getChild(0)); - if (node.getNodeStart() != otherNode.getNodeStart()) { - buf.append("Start time of node (" + node.getNodeStart() + ") " - + "does not match start time of first child " + "(" - + otherNode.getNodeStart() + "), " + "node #" - + otherNode.getSequenceNumber() + ")\n"); - ret = false; - } - if (node.isDone()) { - otherNode = treeIO.readNode(node.getLatestChild()); - if (node.getNodeEnd() != otherNode.getNodeEnd()) { - buf.append("End time of node (" + node.getNodeEnd() - + ") does not match end time of last child (" - + otherNode.getNodeEnd() + ", node #" + try { + /* + * Test that this node's start and end times match the start of the + * first child and the end of the last child, respectively + */ + if (node.getNbChildren() > 0) { + otherNode = treeIO.readNode(node.getChild(0)); + if (node.getNodeStart() != otherNode.getNodeStart()) { + buf.append("Start time of node (" + node.getNodeStart() + ") " + + "does not match start time of first child " + "(" + + otherNode.getNodeStart() + "), " + "node #" + otherNode.getSequenceNumber() + ")\n"); ret = false; } + if (node.isDone()) { + otherNode = treeIO.readNode(node.getLatestChild()); + if (node.getNodeEnd() != otherNode.getNodeEnd()) { + buf.append("End time of node (" + node.getNodeEnd() + + ") does not match end time of last child (" + + otherNode.getNodeEnd() + ", node #" + + otherNode.getSequenceNumber() + ")\n"); + ret = false; + } + } } - } - /* - * Test that the childStartTimes[] array matches the real nodes' start - * times - */ - for (int i = 0; i < node.getNbChildren(); i++) { - otherNode = treeIO.readNode(node.getChild(i)); - if (otherNode.getNodeStart() != node.getChildStart(i)) { - buf.append(" Expected start time of child node #" - + node.getChild(i) + ": " + node.getChildStart(i) - + "\n" + " Actual start time of node #" - + otherNode.getSequenceNumber() + ": " - + otherNode.getNodeStart() + "\n"); - ret = false; + /* + * Test that the childStartTimes[] array matches the real nodes' start + * times + */ + for (int i = 0; i < node.getNbChildren(); i++) { + otherNode = treeIO.readNode(node.getChild(i)); + if (otherNode.getNodeStart() != node.getChildStart(i)) { + buf.append(" Expected start time of child node #" + + node.getChild(i) + ": " + node.getChildStart(i) + + "\n" + " Actual start time of node #" + + otherNode.getSequenceNumber() + ": " + + otherNode.getNodeStart() + "\n"); + ret = false; + } } + + } catch (ClosedChannelException e) { + e.printStackTrace(); } if (!ret) { @@ -582,8 +591,12 @@ class HistoryTree { } void checkIntegrity() { - for (int i = 0; i < nodeCount; i++) { - checkNodeIntegrity(treeIO.readNode(i)); + try { + for (int i = 0; i < nodeCount; i++) { + checkNodeIntegrity(treeIO.readNode(i)); + } + } catch (ClosedChannelException e) { + e.printStackTrace(); } } @@ -622,14 +635,18 @@ class HistoryTree { } curDepth++; - for (i = 0; i < currentNode.getNbChildren(); i++) { - nextNode = treeIO.readNode(currentNode.getChild(i)); - assert (nextNode instanceof CoreNode); // TODO temporary - for (j = 0; j < curDepth - 1; j++) { - writer.print(" "); + try { + for (i = 0; i < currentNode.getNbChildren(); i++) { + nextNode = treeIO.readNode(currentNode.getChild(i)); + assert (nextNode instanceof CoreNode); // TODO temporary + for (j = 0; j < curDepth - 1; j++) { + writer.print(" "); + } + writer.print("+-"); + preOrderPrint(writer, printIntervals, (CoreNode) nextNode); } - writer.print("+-"); - preOrderPrint(writer, printIntervals, (CoreNode) nextNode); + } catch (ClosedChannelException e) { + e.printStackTrace(); } curDepth--; return; @@ -637,7 +654,7 @@ class HistoryTree { /** * Print out the full tree for debugging purposes - * + * * @param writer * PrintWriter in which to write the output * @param printIntervals diff --git a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTreeBackend.java b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTreeBackend.java index d13598022f..e0116a9ec9 100644 --- a/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTreeBackend.java +++ b/org.eclipse.linuxtools.tmf.core/src/org/eclipse/linuxtools/internal/tmf/core/statesystem/historytree/HistoryTreeBackend.java @@ -16,9 +16,11 @@ import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.PrintWriter; +import java.nio.channels.ClosedChannelException; import java.util.List; import org.eclipse.linuxtools.internal.tmf.core.statesystem.IStateHistoryBackend; +import org.eclipse.linuxtools.tmf.core.exceptions.StateSystemDisposedException; import org.eclipse.linuxtools.tmf.core.exceptions.TimeRangeException; import org.eclipse.linuxtools.tmf.core.interval.ITmfStateInterval; import org.eclipse.linuxtools.tmf.core.statevalue.ITmfStateValue; @@ -157,7 +159,7 @@ public class HistoryTreeBackend implements IStateHistoryBackend { @Override public void doQuery(List stateInfo, long t) - throws TimeRangeException { + throws TimeRangeException, StateSystemDisposedException { if (!checkValidTime(t)) { /* We can't possibly have information about this query */ throw new TimeRangeException(); @@ -170,9 +172,13 @@ public class HistoryTreeBackend implements IStateHistoryBackend { currentNode.writeInfoFromNode(stateInfo, t); /* Then we follow the branch down in the relevant children */ - while (currentNode.getNbChildren() > 0) { - currentNode = (CoreNode) sht.selectNextChild(currentNode, t); - currentNode.writeInfoFromNode(stateInfo, t); + try { + while (currentNode.getNbChildren() > 0) { + currentNode = (CoreNode) sht.selectNextChild(currentNode, t); + currentNode.writeInfoFromNode(stateInfo, t); + } + } catch (ClosedChannelException e) { + throw new StateSystemDisposedException(); } /* @@ -184,7 +190,7 @@ public class HistoryTreeBackend implements IStateHistoryBackend { @Override public ITmfStateInterval doSingularQuery(long t, int attributeQuark) - throws TimeRangeException { + throws TimeRangeException, StateSystemDisposedException { return getRelevantInterval(t, attributeQuark); } @@ -202,7 +208,7 @@ public class HistoryTreeBackend implements IStateHistoryBackend { * @return The node containing the information we want */ private HTInterval getRelevantInterval(long t, int key) - throws TimeRangeException { + throws TimeRangeException, StateSystemDisposedException { if (!checkValidTime(t)) { throw new TimeRangeException(); } @@ -212,9 +218,13 @@ public class HistoryTreeBackend implements IStateHistoryBackend { CoreNode currentNode = sht.latestBranch.firstElement(); HTInterval interval = currentNode.getRelevantInterval(key, t); - while (interval == null && currentNode.getNbChildren() > 0) { - currentNode = (CoreNode) sht.selectNextChild(currentNode, t); - interval = currentNode.getRelevantInterval(key, t); + try { + while (interval == null && currentNode.getNbChildren() > 0) { + currentNode = (CoreNode) sht.selectNextChild(currentNode, t); + interval = currentNode.getRelevantInterval(key, t); + } + } catch (ClosedChannelException e) { + throw new StateSystemDisposedException(); } /* * Since we should now have intervals at every attribute/timestamp @@ -252,9 +262,13 @@ public class HistoryTreeBackend implements IStateHistoryBackend { long total = 0; long ret; - for (int seq = 0; seq < sht.getNodeCount(); seq++) { - node = treeIO.readNode(seq); - total += node.getNodeUsagePRC(); + try { + for (int seq = 0; seq < sht.getNodeCount(); seq++) { + node = treeIO.readNode(seq); + total += node.getNodeUsagePRC(); + } + } catch (ClosedChannelException e) { + e.printStackTrace(); } ret = total / sht.getNodeCount(); -- 2.34.1