JCStress static util method check.

Description

Hey, thanks much for looking at this.

I got a potential race condition reported in one of our util classes (the StringUtils class attached here). Only concentrate on the isJSONObject method. Although its not 100% correct the focus on this here is that apparently there is a race condition with the matcher used for the final check. If 2 threads access it, there could be an issue with clear and match (https://www.couchbase.com/issues/browse/SPY-170).

Now I think I already know how to fix it, but I wanted to proof it with JCStress and also try it out.
So I cloned it and fiddled around with it and got it to run. Tests code is also attached.

- I want to pound my StringUtil.isJSONObject very hard to see if the exception described in the ticket is raised. I thought I do it like the other tests, catch it and return -1 not allowing it.

I tried to add a new .xml desc file but somehow it is not picked up, so I attached it to atomic-boolean.xml.. is there something I need to be on the lookout for where to place it?


Now the fun part is that I don't understand the results. It should only be 1 (or even if the logic is wrong) always 0.. and it should hit the -1.. but here is what I get:

FORKED MODE
  Test preset mode: "default"
  Writing the test results to "jcstress.1401173400110"
  Parsing results to "results/"
  Running each test matching ".*StringUtil.*" for 1 forks, 5 iterations, 1000 ms each
  Solo stride size will be autobalanced within [10, 10000] elements
  Hardware threads in use/available: 50/8, yielding was forced, more threads are requested than available.


 (ETA:        n/a) (R: 4.62E+08) (T:   1/1) (F: 1/1) (I: 1/5)   [FAILED] com.couchbase.client.tests.util.StringUtilTest
                     Observed state     Occurrences        Expectation Interpretation      
                             [1, 1] (    1,560,133)         ACCEPTABLE Acceptable to see true.                                     
                             [0, 0] (            1)          FORBIDDEN Other cases are not expected. -1 would mean an exception ...
                             [0, 1] (          136)          FORBIDDEN Other cases are not expected. -1 would mean an exception ...
                             [1, 0] (          160)          FORBIDDEN Other cases are not expected. -1 would mean an exception ...

 (ETA:   00:00:02) (R: 3.34E+06) (T:   1/1) (F: 1/1) (I: 2/5)       [OK] com.couchbase.client.tests.util.StringUtilTest
 (ETA:   00:00:01) (R: 2.44E+06) (T:   1/1) (F: 1/1) (I: 3/5)       [OK] com.couchbase.client.tests.util.StringUtilTest
 (ETA:   00:00:00) (R: 2.16E+06) (T:   1/1) (F: 1/1) (I: 4/5)       [OK] com.couchbase.client.tests.util.StringUtilTest
 (ETA:        now) (R: 2.04E+06) (T:   1/1) (F: 1/1) (I: 5/5)       [OK] com.couchbase.client.tests.util.StringUtilTest
Reading the results back... 
Generating the report... 
Look at results/index.html for the complete run results.

Will throw any pending exceptions at this point.
Exception in thread "main" java.lang.AssertionError: TEST FAILURES: 
com.couchbase.client.tests.util.StringUtilTest: Observed forbidden state: [0, 0]
com.couchbase.client.tests.util.StringUtilTest: Observed forbidden state: [0, 1]
com.couchbase.client.tests.util.StringUtilTest: Observed forbidden state: [1, 0]

	at org.openjdk.jcstress.infra.grading.ExceptionReportPrinter.parse(ExceptionReportPrinter.java:119)
	at org.openjdk.jcstress.JCStress.run(JCStress.java:134)
	at org.openjdk.jcstress.Main.main(Main.java:85)
	


.... I also tried with -c 50 to see if more concurrency hit it.
Any feedback would be awesome, thanks!

StringUtils.java

/**
 * Copyright (C) 2009-2014 Couchbase, Inc.
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to deal
 * in the Software without restriction, including without limitation the rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in
 * all copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALING
 * IN THE SOFTWARE.
 */

package net.spy.memcached.util;

import net.spy.memcached.KeyUtil;
import net.spy.memcached.MemcachedClientIF;

import java.util.Collection;
import java.util.Iterator;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
 * Utility methods on string objects.
 */
public final class StringUtils {

  /**
   * A pattern to match on a signed integer value.
   */
  private static final Pattern decimalPattern = Pattern.compile("^-?\\d+$");

  /**
   * The matcher for the decimal pattern regex.
   */
  private static final Matcher decimalMatcher = decimalPattern.matcher("");

  /**
   * Maximum supported key length.
   */
  private static final int MAX_KEY_LENGTH = MemcachedClientIF.MAX_KEY_LENGTH;

  /**
   * Exception thrown if the input key is too long.
   */
  private static final IllegalArgumentException KEY_TOO_LONG_EXCEPTION =
    new IllegalArgumentException("Key is too long (maxlen = "
      + MAX_KEY_LENGTH + ")");

  /**
   * Exception thrown if the input key is empty.
   */
  private static final IllegalArgumentException KEY_EMPTY_EXCEPTION =
    new IllegalArgumentException("Key must contain at least one character.");

  /**
   * Preset the stack traces for the static exceptions.
   */
  static {
    KEY_TOO_LONG_EXCEPTION.setStackTrace(new StackTraceElement[0]);
    KEY_EMPTY_EXCEPTION.setStackTrace(new StackTraceElement[0]);
  }

  /**
   * Private constructor, since this is a purely static class.
   */
  private StringUtils() {
    throw new UnsupportedOperationException();
  }

  /**
   * Join a collection of strings together into one.
   *
   * @param chunks the chunks to join.
   * @param delimiter the delimiter between the keys.
   * @return the fully joined string.
   */
  public static String join(final Collection<String> chunks,
    final String delimiter) {
    StringBuilder sb = new StringBuilder();
    if (!chunks.isEmpty()) {
      Iterator<String> itr = chunks.iterator();
      sb.append(itr.next());
      while (itr.hasNext()) {
        sb.append(delimiter);
        sb.append(itr.next());
      }
    }
    return sb.toString();
  }

  /**
   * Check if a given string is a JSON object.
   *
   * @param s the input string.
   * @return true if it is a JSON object, false otherwise.
   */
  public static boolean isJsonObject(final String s) {
    if (s == null || s.isEmpty()) {
      return false;
    }

    if (s.startsWith("{") || s.startsWith("[")
      || "true".equals(s) || "false".equals(s)
      || "null".equals(s) || decimalMatcher.reset(s).matches()) {
      return true;
    }

    return false;
  }

  /**
   * Check if a given key is valid to transmit.
   *
   * @param key the key to check.
   * @param binary if binary protocol is used.
   */
  public static void validateKey(final String key, final boolean binary) {
    byte[] keyBytes = KeyUtil.getKeyBytes(key);
    int keyLength = keyBytes.length;

    if (keyLength > MAX_KEY_LENGTH) {
      throw KEY_TOO_LONG_EXCEPTION;
    }

    if (keyLength == 0) {
      throw KEY_EMPTY_EXCEPTION;
    }

    if(!binary) {
      for (byte b : keyBytes) {
        if (b == ' ' || b == '\n' || b == '\r' || b == 0) {
          throw new IllegalArgumentException(
              "Key contains invalid characters:  ``" + key + "''");
        }
      }
    }

  }
}

StringUtilTest.java

/*
 * Copyright (c) 2014, 2014, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.  Oracle designates this
 * particular file as subject to the "Classpath" exception as provided
 * by Oracle in the LICENSE file that accompanied this code.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */
package com.couchbase.client.tests.util;

import net.spy.memcached.util.StringUtils;
import org.openjdk.jcstress.annotations.Actor;
import org.openjdk.jcstress.annotations.JCStressTest;
import org.openjdk.jcstress.annotations.State;
import org.openjdk.jcstress.infra.results.IntResult2;

@JCStressTest
@State
public class StringUtilTest {

    @Actor
    public void actor1(IntResult2 result) {
        try {
            result.r1 = StringUtils.isJsonObject("1234") ? 1 : 0;
        } catch(Exception ex) {
            result.r1 = -1;
        }
    }

    @Actor
    public void actor2(IntResult2 result) {
        try {
            result.r2 = StringUtils.isJsonObject("5678") ? 1 : 0;
        } catch(Exception ex) {
            result.r2 = -1;
        }
    }

}

}

test.xml

    <test name="com.couchbase.client.tests.util.StringUtilTest">
        <contributed-by>Michael Nitschinger (michael.nitschinger@couchbase.com)</contributed-by>
        <description>
            Tests the thread-safeness of the StringUtil class.
        </description>
        <case>
            <match>[1, 1]</match>
            <expect>ACCEPTABLE</expect>
            <description>
                Acceptable to see true.
            </description>
        </case>
        <unmatched>
            <expect>FORBIDDEN</expect>
            <description>
                Other cases are not expected. -1 would mean an exception raised.
            </description>
        </unmatched>
    </test>