Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.11.0, 2.11.1, 2.11.2
    • Fix Version/s: 2.11.3
    • Component/s: None
    • Security Level: Public
    • Labels:
      None

      Description

      I found a multi-threading issue introduced in SPY-127. I added my comments already into this link but pasting it here as well.

      https://github.com/couchbase/spymemcached/commit/d8cd4e61226ce5f48f1aab8e94602f86defce36f#commitcomment-6452097

      -------

      The non-thread safe code has to do with "decimalMatcher.reset(s).matches()".

      When one thread is in the "matches", and then a second thread calls reset, I get a runtime exception..

      java.lang.StringIndexOutOfBoundsException: String index out of range: 4
      at java.lang.String.charAt(String.java:658)
      at java.util.regex.Pattern$BmpCharProperty.match(Pattern.java:3715)
      at java.util.regex.Pattern$Curly.match0(Pattern.java:4158)
      at java.util.regex.Pattern$Curly.match(Pattern.java:4132)
      at java.util.regex.Pattern$Ques.match(Pattern.java:4079)
      at java.util.regex.Pattern$Begin.match(Pattern.java:3472)
      at java.util.regex.Matcher.match(Matcher.java:1221)
      at java.util.regex.Matcher.matches(Matcher.java:559)
      at net.spy.memcached.util.StringUtils.isJsonObject(StringUtils.java:113)
      at net.spy.memcached.transcoders.SerializingTranscoder.encode(SerializingTranscoder.java:134)
      at net.spy.memcached.MemcachedClient.asyncStore(MemcachedClient.java:305)
      at net.spy.memcached.MemcachedClient.set(MemcachedClient.java:929)
      at com.couchbase.client.CouchbaseClient.set(CouchbaseClient.java:1266)
      at com.example.Test$1.run(Test.java:124)
      at java.lang.Thread.run(Thread.java:724)

      To reproduce, just create for example 10 threads that each loop 100K times calling set("anykey","123456789"). i.e. 1M set operations distributed between 10 threads.

      I encountered this because I have a very fast counter keeping track of my position in a set of objects.

      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        daschl Michael Nitschinger added a comment -

        Makes sense, what do you think about a thread local since I don't want to introduce synchronization in this place?

        Show
        daschl Michael Nitschinger added a comment - Makes sense, what do you think about a thread local since I don't want to introduce synchronization in this place?
        Hide
        nkavian Nas Kavian added a comment -

        I also considered thread local but came to the conclusion that it would essentially be equal to a memory leak. For example, I have 4 static clients connected to 1 bucket each. I have background threads created dynamically performing work. Hypothetically if I am creating 10 threads per minute, thread local will just expand over time and not shrink until the clients are shutdown.

        From what I've read, I would just move the matcher directly into the method itself:
        decimalPattern.matcher("").reset(s).matches()
        instead of
        decimalMatcher.reset(s).matches()

        i.e. Pattern is thread safe but not Matcher:
        http://www.javamex.com/tutorials/regular_expressions/thread_safety.shtml#.U4QmFZRdVmo
        http://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html

        I'm hoping performance is just as similar because this still avoids the expensive try/catch control flow.

        Show
        nkavian Nas Kavian added a comment - I also considered thread local but came to the conclusion that it would essentially be equal to a memory leak. For example, I have 4 static clients connected to 1 bucket each. I have background threads created dynamically performing work. Hypothetically if I am creating 10 threads per minute, thread local will just expand over time and not shrink until the clients are shutdown. From what I've read, I would just move the matcher directly into the method itself: decimalPattern.matcher("").reset(s).matches() instead of decimalMatcher.reset(s).matches() i.e. Pattern is thread safe but not Matcher: http://www.javamex.com/tutorials/regular_expressions/thread_safety.shtml#.U4QmFZRdVmo http://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html I'm hoping performance is just as similar because this still avoids the expensive try/catch control flow.
        Hide
        daschl Michael Nitschinger added a comment -

        Btw, I'm moving this to SPY since its part of the spymemcached library.

        Show
        daschl Michael Nitschinger added a comment - Btw, I'm moving this to SPY since its part of the spymemcached library.
        Hide
        daschl Michael Nitschinger added a comment -

        Good point, yeah. I'll go ahead and look into it a bit more. Do you have some sample code at hand that shows the bug?

        Show
        daschl Michael Nitschinger added a comment - Good point, yeah. I'll go ahead and look into it a bit more. Do you have some sample code at hand that shows the bug?
        Hide
        daschl Michael Nitschinger added a comment -

        I think we can go with decimalPattern.matcher(s).matches(), since we dont need to reset it if it comes from a fresh matcher

        Show
        daschl Michael Nitschinger added a comment - I think we can go with decimalPattern.matcher(s).matches(), since we dont need to reset it if it comes from a fresh matcher
        Hide
        daschl Michael Nitschinger added a comment -

        Okay cool with JCStress and some help of the jdk core team I managed to reproduce and fix (and verify the fix). Did some great learning as well around

        https://gist.github.com/daschl/95c99def24cd0904dc45

        Proper fix is: decimalPattern.matcher(s).matches()

        I'll get that into 2.11.3 (and therefor 1.4.2)

        Show
        daschl Michael Nitschinger added a comment - Okay cool with JCStress and some help of the jdk core team I managed to reproduce and fix (and verify the fix). Did some great learning as well around https://gist.github.com/daschl/95c99def24cd0904dc45 Proper fix is: decimalPattern.matcher(s).matches() I'll get that into 2.11.3 (and therefor 1.4.2)

          People

          • Assignee:
            daschl Michael Nitschinger
            Reporter:
            nkavian Nas Kavian
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Gerrit Reviews

              There are no open Gerrit changes