Gracefully handle parse failure with locking #4114
Conversation
Before we would poison the map with the lock object so the second caller would get a weird ClassCastException.
| } catch (Throwable e) { | ||
| // Remove the lock on failure. Any other locked threads will retry as a result. | ||
| serviceMethodCache.remove(method); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
Change is here
| if (result == null) { | ||
| // The other thread failed its parsing. We will retry (and probably also fail). | ||
| continue; | ||
| } |
There was a problem hiding this comment.
And here (plus the enclosing while (true))
There was a problem hiding this comment.
I am thinking about whether this situation is possible: thread 1 put its lock1to the cache, granted the right to parse the method, but failed and threw an exception. Then, thread 2, which was waiting for thread 1 to parse the method, synchronize lock1 successfully, find the result is null, continue the while loop and put its lock2 to the cache. Then, Thread 3 synchronize lock1. However, the result retrieved from the cache by thread 3 is lock2 instead of null. Thus, thread 3 try to cast object to ServiceMethod and a ClassCastException happens. Can we check result instanceof ServiceMethod<?> here? What's more, can we check lookup instanceof ServiceMethod<?> before synchronize lookup to eliminate the pointless lock and redundant lookup you've metioned in comment(line 253)? Thanks.
Before we would poison the map with the lock object so the second caller would get a weird ClassCastException.
Closes #4113
CHANGELOG.md's "Unreleased" section has been updated, if applicable.