Merge pull request #790 from sebright/deprecate-Tags-setState

Deprecate Tags.setState, and throw an exception when it is called after getState.

5 files changed, 65 insertions, 20 deletions

diff --git a/api/src/main/java/io/opencensus/tags/NoopTags.java b/api/src/main/java/io/opencensus/tags/NoopTags.java
index b7c86a2d..973a1b40 100644
--- a/api/src/main/java/io/opencensus/tags/NoopTags.java
+++ b/api/src/main/java/io/opencensus/tags/NoopTags.java
@@ -26,6 +26,7 @@ import io.opencensus.tags.propagation.TagPropagationComponent;
 import java.util.Collections;
 import java.util.Iterator;
 import javax.annotation.concurrent.Immutable;
+import javax.annotation.concurrent.ThreadSafe;
 
 /** No-op implementations of tagging classes. */
 final class NoopTags {
@@ -37,8 +38,8 @@ final class NoopTags {
    *
    * @return a {@code TagsComponent} that has a no-op implementation for {@code Tagger}.
    */
-  static TagsComponent getNoopTagsComponent() {
-    return NoopTagsComponent.INSTANCE;
+  static TagsComponent newNoopTagsComponent() {
+    return new NoopTagsComponent();
   }
 
   /**
@@ -81,9 +82,9 @@ final class NoopTags {
     return NoopTagContextBinarySerializer.INSTANCE;
   }
 
-  @Immutable
+  @ThreadSafe
   private static final class NoopTagsComponent extends TagsComponent {
-    static final TagsComponent INSTANCE = new NoopTagsComponent();
+    private volatile boolean isRead;
 
     @Override
     public Tagger getTagger() {
@@ -97,12 +98,15 @@ final class NoopTags {
 
     @Override
     public TaggingState getState() {
+      isRead = true;
       return TaggingState.DISABLED;
     }
 
     @Override
+    @Deprecated
     public void setState(TaggingState state) {
       Preconditions.checkNotNull(state, "state");
+      Preconditions.checkState(!isRead, "State was already read, cannot set state.");
     }
   }
 
diff --git a/api/src/main/java/io/opencensus/tags/Tags.java b/api/src/main/java/io/opencensus/tags/Tags.java
index 5378fa5a..593312ae 100644
--- a/api/src/main/java/io/opencensus/tags/Tags.java
+++ b/api/src/main/java/io/opencensus/tags/Tags.java
@@ -55,6 +55,9 @@ public final class Tags {
    * <p>When no implementation is available, {@code getState} always returns {@link
    * TaggingState#DISABLED}.
    *
+   * <p>Once {@link #getState()} is called, subsequent calls to {@link #setState(TaggingState)} will
+   * throw an {@code IllegalStateException}.
+   *
    * @return the current {@code TaggingState}.
    */
   public static TaggingState getState() {
@@ -64,10 +67,16 @@ public final class Tags {
   /**
    * Sets the current {@code TaggingState}.
    *
-   * <p>When no implementation is available, {@code setState} has no effect.
+   * <p>When no implementation is available, {@code setState} does not change the state.
    *
    * @param state the new {@code TaggingState}.
+   * @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 {@link #getState()}
+   *     has been called, in order to limit changes to the result of {@code getState()}.
    */
+  @Deprecated
   public static void setState(TaggingState state) {
     tagsComponent.setState(state);
   }
@@ -99,6 +108,6 @@ public final class Tags {
               + "default implementation for TagsComponent.",
           e);
     }
-    return NoopTags.getNoopTagsComponent();
+    return NoopTags.newNoopTagsComponent();
   }
 }
diff --git a/api/src/main/java/io/opencensus/tags/TagsComponent.java b/api/src/main/java/io/opencensus/tags/TagsComponent.java
index 755173d8..b0965077 100644
--- a/api/src/main/java/io/opencensus/tags/TagsComponent.java
+++ b/api/src/main/java/io/opencensus/tags/TagsComponent.java
@@ -37,6 +37,9 @@ public abstract class TagsComponent {
    * <p>When no implementation is available, {@code getState} always returns {@link
    * TaggingState#DISABLED}.
    *
+   * <p>Once {@link #getState()} is called, subsequent calls to {@link #setState(TaggingState)} will
+   * throw an {@code IllegalStateException}.
+   *
    * @return the current {@code TaggingState}.
    */
   public abstract TaggingState getState();
@@ -44,9 +47,15 @@ public abstract class TagsComponent {
   /**
    * Sets the current {@code TaggingState}.
    *
-   * <p>When no implementation is available, {@code setState} has no effect.
+   * <p>When no implementation is available, {@code setState} does not change the state.
    *
    * @param state the new {@code TaggingState}.
+   * @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 limit changes to the result of {@code getState()}.
    */
+  @Deprecated
   public abstract void setState(TaggingState state);
 }
diff --git a/api/src/test/java/io/opencensus/tags/NoopTagsTest.java b/api/src/test/java/io/opencensus/tags/NoopTagsTest.java
index 8434bd83..db07520e 100644
--- a/api/src/test/java/io/opencensus/tags/NoopTagsTest.java
+++ b/api/src/test/java/io/opencensus/tags/NoopTagsTest.java
@@ -50,19 +50,42 @@ public final class NoopTagsTest {
 
   @Test
   public void noopTagsComponent() {
-    assertThat(NoopTags.getNoopTagsComponent().getTagger()).isSameAs(NoopTags.getNoopTagger());
-    assertThat(NoopTags.getNoopTagsComponent().getTagPropagationComponent())
+    assertThat(NoopTags.newNoopTagsComponent().getTagger()).isSameAs(NoopTags.getNoopTagger());
+    assertThat(NoopTags.newNoopTagsComponent().getTagPropagationComponent())
         .isSameAs(NoopTags.getNoopTagPropagationComponent());
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void noopTagsComponent_SetState_DisallowsNull() {
-    TagsComponent noopTagsComponent = NoopTags.getNoopTagsComponent();
+    TagsComponent noopTagsComponent = NoopTags.newNoopTagsComponent();
     thrown.expect(NullPointerException.class);
     noopTagsComponent.setState(null);
   }
 
   @Test
+  @SuppressWarnings("deprecation")
+  public void preventSettingStateAfterGettingState_DifferentState() {
+    TagsComponent noopTagsComponent = NoopTags.newNoopTagsComponent();
+    noopTagsComponent.setState(TaggingState.DISABLED);
+    noopTagsComponent.getState();
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("State was already read, cannot set state.");
+    noopTagsComponent.setState(TaggingState.ENABLED);
+  }
+
+  @Test
+  @SuppressWarnings("deprecation")
+  public void preventSettingStateAfterGettingState_SameState() {
+    TagsComponent noopTagsComponent = NoopTags.newNoopTagsComponent();
+    noopTagsComponent.setState(TaggingState.DISABLED);
+    noopTagsComponent.getState();
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("State was already read, cannot set state.");
+    noopTagsComponent.setState(TaggingState.DISABLED);
+  }
+
+  @Test
   public void noopTagger() {
     Tagger noopTagger = NoopTags.getNoopTagger();
     assertThat(noopTagger.empty()).isSameAs(NoopTags.getNoopTagContext());
diff --git a/api/src/test/java/io/opencensus/tags/TagsTest.java b/api/src/test/java/io/opencensus/tags/TagsTest.java
index e87407db..dee517b6 100644
--- a/api/src/test/java/io/opencensus/tags/TagsTest.java
+++ b/api/src/test/java/io/opencensus/tags/TagsTest.java
@@ -56,20 +56,20 @@ public class TagsTest {
         .isEqualTo("io.opencensus.tags.NoopTags$NoopTagsComponent");
   }
 
+  // There is only one test that modifies tagging state in the Tags class, since the state is
+  // global, and it could affect other tests. NoopTagsTest has more thorough tests for tagging
+  // state.
   @Test
-  public void getState() {
-    assertThat(Tags.getState()).isEqualTo(TaggingState.DISABLED);
-  }
-
-  @Test
-  public void setState_IgnoresInput() {
+  @SuppressWarnings("deprecation")
+  public void testState() {
+    // Test that setState ignores its input.
     Tags.setState(TaggingState.ENABLED);
     assertThat(Tags.getState()).isEqualTo(TaggingState.DISABLED);
-  }
 
-  @Test(expected = NullPointerException.class)
-  public void setState_DisallowsNull() {
-    Tags.setState(null);
+    // Test that setState cannot be called after getState.
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("State was already read, cannot set state.");
+    Tags.setState(TaggingState.ENABLED);
   }
 
   @Test