Deprecate Stats.setState, and throw an exception when it is called after getState. (#792)

* Throw IllegalStateException if setState is called after getState

* Deprecate Stats.setState

* Remove a test that has potential race condition.

* Remove Deprecated from impl

5 files changed, 43 insertions, 3 deletions

diff --git a/api/src/main/java/io/opencensus/stats/NoopStats.java b/api/src/main/java/io/opencensus/stats/NoopStats.java
index a016e2e9..ac9329f6 100644
--- a/api/src/main/java/io/opencensus/stats/NoopStats.java
+++ b/api/src/main/java/io/opencensus/stats/NoopStats.java
@@ -18,6 +18,7 @@ package io.opencensus.stats;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Maps;
@@ -83,6 +84,7 @@ final class NoopStats {
   @ThreadSafe
   private static final class NoopStatsComponent extends StatsComponent {
     private final ViewManager viewManager = newNoopViewManager();
+    private volatile boolean isRead;
 
     @Override
     public ViewManager getViewManager() {
@@ -96,12 +98,15 @@ final class NoopStats {
 
     @Override
     public StatsCollectionState getState() {
+      isRead = true;
       return StatsCollectionState.DISABLED;
     }
 
     @Override
+    @Deprecated
     public void setState(StatsCollectionState state) {
       Preconditions.checkNotNull(state, "state");
+      checkState(!isRead, "State was already read, cannot set state.");
     }
   }
 
diff --git a/api/src/main/java/io/opencensus/stats/Stats.java b/api/src/main/java/io/opencensus/stats/Stats.java
index 016455e9..3bc78640 100644
--- a/api/src/main/java/io/opencensus/stats/Stats.java
+++ b/api/src/main/java/io/opencensus/stats/Stats.java
@@ -44,6 +44,9 @@ public final class Stats {
    * <p>When no implementation is available, {@code getState} always returns {@link
    * StatsCollectionState#DISABLED}.
    *
+   * <p>Once {@link #getState()} is called, subsequent calls to {@link
+   * #setState(StatsCollectionState)} will throw an {@code IllegalStateException}.
+   *
    * @return the current {@code StatsCollectionState}.
    */
   public static StatsCollectionState getState() {
@@ -53,10 +56,19 @@ public final class Stats {
   /**
    * Sets the current {@code StatsCollectionState}.
    *
-   * <p>When no implementation is available, {@code setState} has no effect.
+   * <p>When no implementation is available, {@code setState} does not change the state.
+   *
+   * <p>If state is set to {@link StatsCollectionState#DISABLED}, all stats that are previously
+   * recorded will be cleared.
    *
    * @param state the new {@code StatsCollectionState}.
+   * @throws IllegalStateException if {@link #getState()} was previously called.
+   * @deprecated This method is deprecated because other libraries could cache the result of {@link
+   *     #getState()}, use a stale value, and behave incorrectly. It is only safe to call early in
+   *     initialization. This method throws {@link IllegalStateException} after {@code getState()}
+   *     has been called, in order to prevent the result of {@code getState()} from changing.
    */
+  @Deprecated
   public static void setState(StatsCollectionState state) {
     statsComponent.setState(state);
   }
diff --git a/api/src/main/java/io/opencensus/stats/StatsComponent.java b/api/src/main/java/io/opencensus/stats/StatsComponent.java
index cdcfc3ee..30f56a91 100644
--- a/api/src/main/java/io/opencensus/stats/StatsComponent.java
+++ b/api/src/main/java/io/opencensus/stats/StatsComponent.java
@@ -45,12 +45,18 @@ public abstract class StatsComponent {
   /**
    * Sets the current {@code StatsCollectionState}.
    *
-   * <p>When no implementation is available, {@code setState} has no effect.
+   * <p>When no implementation is available, {@code setState} does not change the state.
    *
    * <p>If state is set to {@link StatsCollectionState#DISABLED}, all stats that are previously
    * recorded will be cleared.
    *
    * @param state the new {@code StatsCollectionState}.
+   * @throws IllegalStateException if {@link #getState()} was previously called.
+   * @deprecated This method is deprecated because other libraries could cache the result of {@link
+   *     #getState()}, use a stale value, and behave incorrectly. It is only safe to call early in
+   *     initialization. This method throws {@link IllegalStateException} after {@code getState()}
+   *     has been called, in order to prevent the result of {@code getState()} from changing.
    */
+  @Deprecated
   public abstract void setState(StatsCollectionState state);
 }
diff --git a/api/src/test/java/io/opencensus/stats/NoopStatsTest.java b/api/src/test/java/io/opencensus/stats/NoopStatsTest.java
index 9a52b7db..b3f81715 100644
--- a/api/src/test/java/io/opencensus/stats/NoopStatsTest.java
+++ b/api/src/test/java/io/opencensus/stats/NoopStatsTest.java
@@ -67,6 +67,7 @@ public final class NoopStatsTest {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void noopStatsComponent_SetState_IgnoresInput() {
     StatsComponent noopStatsComponent = NoopStats.newNoopStatsComponent();
     noopStatsComponent.setState(StatsCollectionState.ENABLED);
@@ -74,12 +75,24 @@ public final class NoopStatsTest {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void noopStatsComponent_SetState_DisallowsNull() {
     StatsComponent noopStatsComponent = NoopStats.newNoopStatsComponent();
     thrown.expect(NullPointerException.class);
     noopStatsComponent.setState(null);
   }
 
+  @Test
+  @SuppressWarnings("deprecation")
+  public void noopStatsComponent_DisallowsSetStateAfterGetState() {
+    StatsComponent noopStatsComponent = NoopStats.newNoopStatsComponent();
+    noopStatsComponent.setState(StatsCollectionState.DISABLED);
+    noopStatsComponent.getState();
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("State was already read, cannot set state.");
+    noopStatsComponent.setState(StatsCollectionState.ENABLED);
+  }
+
   // The NoopStatsRecorder should do nothing, so this test just checks that record doesn't throw an
   // exception.
   @Test
diff --git a/api/src/test/java/io/opencensus/stats/StatsTest.java b/api/src/test/java/io/opencensus/stats/StatsTest.java
index cc60d6c0..4219173a 100644
--- a/api/src/test/java/io/opencensus/stats/StatsTest.java
+++ b/api/src/test/java/io/opencensus/stats/StatsTest.java
@@ -69,13 +69,17 @@ public final class StatsTest {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void setState_IgnoresInput() {
     Stats.setState(StatsCollectionState.ENABLED);
     assertThat(Stats.getState()).isEqualTo(StatsCollectionState.DISABLED);
   }
 
-  @Test(expected = NullPointerException.class)
+  @Test
+  @SuppressWarnings("deprecation")
   public void setState_DisallowsNull() {
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("state");
     Stats.setState(null);
   }
 }