Question on stdlib cache?

[ramith]

Description:
So I was looking at the cache implementation at :
https://github.com/ballerina-platform/ballerina-lang/blob/release-1.0.1/stdlib/cache/src/main/ballerina/src/cache/cache.bal

I have following questions:

1. I see there is synchronization in put() function but not in get function. is this intentional?
2. Further, if you look at line 173 to 175 (following code fragment), I would argue there can be a put() operation which could happen in parallel busting the logic. e.g. think about a scenario where !self.hasKey(key) becomes true and a put() happens before the return ();

      // Check whether the requested cache is available.
        if (!self.hasKey(key)) {
            return ();
        }

3. I suppose we can deduce a couple of more such scenarios just by looking at get() and put()

But I could be wrong given that I don't have a clear understanding of how ballerina's concurrency is handled.

Steps to reproduce:
N/A
Affected Versions:
v1.0, v 1.0.1
OS, DB, other environment details and versions:
MacOS, Jdk 1.8

Suggested Assignees (optional):
@wggihan @pubudu91 @a5anka

[a5anka]

Another issue is that we are locking all the put() functions of multiple caches using the global object cacheMap. This adds an unnecessary performance overhead.

[wggihan]

Description:
So I was looking at the cache implementation at :
https://github.com/ballerina-platform/ballerina-lang/blob/release-1.0.1/stdlib/cache/src/main/ballerina/src/cache/cache.bal

I have following questions:

1. I see there is synchronization in put() function but not in get function. is this intentional?
   Yes, this is intentional. The main idea behind the reason is to improve cache performance. Basically get doesn't need any handling in term of concurrently accessing.

2. Further, if you look at line 173 to 175 (following code fragment), I would argue there can be a put() operation which could happen in parallel busting the logic. e.g. think about a scenario where !self.hasKey(key) becomes true and a put() happens before the return ();

      // Check whether the requested cache is available.
        if (!self.hasKey(key)) {
            return ();
        }

IINM, cache misses can happen due to this. But since this is a simple cache. I believe that is acceptable.

1. I suppose we can deduce a couple of more such scenarios just by looking at get() and put()

But I could be wrong given that I don't have a clear understanding of how ballerina's concurrency is handled.

Steps to reproduce:
N/A
Affected Versions:
v1.0, v 1.0.1
OS, DB, other environment details and versions:
MacOS, Jdk 1.8

Suggested Assignees (optional):
@wggihan @pubudu91 @a5anka

[wggihan]

Another issue is that we are locking all the put() functions of multiple caches using the global object cacheMap. This adds an unnecessary performance overhead.

With the current implementation, we can't omit the global cache map.

[chethiya]

I assume original post is regarding data corruption that can occur due to race condition. But the given example there (i.e. hasKey) doesn't cause any such race conditions it seems. Ballerina map<> implementation seems to be thread safe (assuming it's implemented using MapValueImpl class)

But I think following line in get() of Cache can cause data races

            cacheEntry.lastAccessedTime = time:currentTime().time;

So this can cause cache to evict wrong keys in case the values in cacheEntry.lastAccessedTime get corrupted due to data races.