Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ConcurrentModificationException for getStats Op #23

Closed
wants to merge 1 commit into from

Conversation

amcrn
Copy link

@amcrn amcrn commented Nov 2, 2017

If the result of MemcachedClient.getStats() is immediately iterated
over, a ConcurrentModificationException can be thrown if the
operationTimeout value has been exceeded before mutations to the
Map have completed.

Example:

  • blatch.await(operationTimeout, TimeUnit.MILLISECONDS); returns
    false after waiting operationTimeout, resulting in
    MemcachedClient.getStats() returning the rv Map.
  • While the caller starts to iterate over the Map, the
    StatsOperation.Callback's gotStat mutates the Map, resulting in
    a ConcurrentModificationException.

How to Reproduce:

public static void main(String[] args) throws Exception {
  ConnectionFactory connectionFactory = new ConnectionFactoryBuilder()
    .setProtocol(Protocol.TEXT)
    .setFailureMode(FailureMode.Cancel)
    .setOpTimeout(1)
    .build();
  MemcachedClient client = new MemcachedClient(
    connectionFactory, AddrUtil.getAddresses("localhost:11211"));
  while(true) {
    Map<SocketAddress, Map<String, String>> allStats = client.getStats();
    if (allStats.isEmpty()) continue;
    Map<String, String> stats = allStats.entrySet().iterator().next().getValue();
    for(Entry<String,String> stat : stats.entrySet()) {
      System.out.println(stat.getKey() + " => " + stat.getValue());
    }
  }
}

Output:

2017-11-02 14:37:52.269 WARN net.spy.memcached.MemcachedClient:  Unsuccessful stat fetch: {OperationStatus success=false:  timed out}
Exception in thread "main" java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:922)
	at java.util.HashMap$EntryIterator.next(HashMap.java:962)
	at java.util.HashMap$EntryIterator.next(HashMap.java:960)
	at com.xxx.xxx.xxx.xxx.xxx.StatsBug.main(StatsBug.java:31)

Notes:

  • Written in a way to be completely backwards compatible.
  • Guarantees that the caller only sees the entire stats result for a
    SocketAddress, or no result at all. Converting the Maps to
    ConcurrentHashMaps alone would result in partially complete
    Maps.

If the result of MemcachedClient.getStats() is immediately iterated
over, a ConcurrentModificationException can be thrown if the
operationTimeout value has been exceeded before mutations to the
Map have completed.

Example:

* blatch.await(operationTimeout, TimeUnit.MILLISECONDS); returns
  false after waiting `operationTimeout`, resulting in
  MemcachedClient.getStats() returning the `rv` Map.
* While the caller starts to iterate over the Map, the
  StatsOperation.Callback's gotStat mutates the Map, resulting in
  a ConcurrentModificationException.

How to Reproduce:

```
public static void main(String[] args) throws Exception {
  ConnectionFactory connectionFactory = new ConnectionFactoryBuilder()
    .setProtocol(Protocol.TEXT)
    .setFailureMode(FailureMode.Cancel)
    .setOpTimeout(1)
    .build();
  MemcachedClient client = new MemcachedClient(
    connectionFactory, AddrUtil.getAddresses("localhost:11211"));
  while(true) {
    Map<SocketAddress, Map<String, String>> allStats = client.getStats();
    if (allStats.isEmpty()) continue;
    Map<String, String> stats = allStats.entrySet().iterator().next().getValue();
    for(Entry<String,String> stat : stats.entrySet()) {
      System.out.println(stat.getKey() + " => " + stat.getValue());
    }
  }
}
```

Output:

```
2017-11-02 14:37:52.269 WARN net.spy.memcached.MemcachedClient:  Unsuccessful stat fetch: {OperationStatus success=false:  timed out}
Exception in thread "main" java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:922)
	at java.util.HashMap$EntryIterator.next(HashMap.java:962)
	at java.util.HashMap$EntryIterator.next(HashMap.java:960)
	at com.xxx.xxx.xxx.xxx.xxx.StatsBug.main(StatsBug.java:31)
  ```

Notes:

* Written in a way to be completely backwards compatible.
* Guarantees that the caller only sees the entire stats result for a
  SocketAddress, or no result at all. Converting the Maps to
  ConcurrentHashMaps alone would result in partially complete
  Maps.
@ingenthr ingenthr self-assigned this Nov 2, 2017
@ingenthr ingenthr requested a review from daschl November 2, 2017 22:49
@amcrn
Copy link
Author

amcrn commented Dec 1, 2017

@ingenthr @daschl Hey there, checking in to see if there's been any progress in reviewing this (and the other open) pull requests? Thanks!

@ingenthr
Copy link

ingenthr commented Dec 1, 2017

hey @amcrn - sorry for the delay, haven't forgotten but also haven't gotten to look recently. Thanks for the ping though-- will look asap!

@amcrn
Copy link
Author

amcrn commented Jan 16, 2018

@ingenthr @daschl Happy 2018! Any chance you'll have an opportunity to look at this pull request (and the others in the queue) in the coming week(s)? Thanks.

@ingenthr
Copy link

@amcrn was just talking about this yesterday with @daschl. my December ambitions were quashed by other people's vacations, but yep, we want to move these forward soon. sorry for the delay and thanks for the patience!

@amcrn
Copy link
Author

amcrn commented Mar 19, 2018

@ingenthr Re-bumping given that it's been about 2 months since your last reply. Thanks!

@ingenthr
Copy link

Man, I suck. Sorry about that @amcrn. Been busy with the vulcan release lately... but I should stop offering excuses. I just need to dedicate some time. I've just blocked off some time in the calendar to work through this later this week. Bear with me!

@ingenthr
Copy link

ingenthr commented Apr 2, 2018

Merged over at http://review.couchbase.org/#/c/91953/

@ingenthr ingenthr closed this Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants