Added audio cache for wav tracks

[achikin]

In the current implementation, Restcomm caches audio files in application server, while media server downloads them from application server every time it plays a file to a user. It's ok to do so when you keep app server and meida server on the same machine. As soon as we have split app and media server - we've added cache on mediaserver's side. We've used http://www.ehcache.org/ not to reinvent the wheel.

[deruelle]

Thanks @achikin ! @hrosa please review.

[hrosa]

would it make sense to add an option to enable/disable caching?

[hrosa]

@achikin , I've added some comments in your commit.

Also, can you write test case to verify the cache well-functioning in the WavTrack class?
Were you able to do some performance tests using the cache? If so, can you comment on performance improvements?

Thank you for your contribution!

[hamsterksu]

hi @hrosa @jaimecasero

i have made a few changes:

1. AudioCache was renamed to RemoteStreamProvider.
   we have 2 implementation with cache and w/o it
2. WavTrackCacheTest was added to be sure that wavtrack works with and w/o cache
3. I have added synchronization (ReentrantLock) to CachedRemoteStreamProvider
4. poolSize was moved back to player tag to save uniformity

Could you review PR please?

[hrosa]

@hamsterksu no way around that lock on every access to the singleton cache?

[hamsterksu]

@hrosa
oh, stream reading is still a blocker for cache. I have 2 ideas how to fix it. I will do it today

[hrosa]

doesn't cache support putIfAbsent? That would be preferred to locking. It should also be OK now since stream is now lazily loaded.

[hamsterksu]

yes, it's possible to replace the following code by putIfAbsent but i use it to avoid unnecessary objects creating

lock.lock();
try {
    //need to check twice
    if (!cache.containsKey(uri)) {
        cache.put(uri, new AudioStreamCache(uri));
    }
} finally {
    lock.unlock();
}

[hrosa]

I'm really worried about bottleneck imposed by this lock. Let's remove it and use putIfAbsent.
We can workaround the object creation issue by recycling them using a pool. What do you think @hamsterksu ?

Object old = putIfAbsent(K, V)
if(old != null) then recycle(V)

[hrosa]

when().thenReturn() form is preferred

[hamsterksu]

we need to use this variant. otherwise answer will be cached and inputstream can't be read twice

[hrosa]

Hi @hamsterksu can you pool the AudioStreamCache objects? I'm also thinking pool size should be configurable (via config file) so users can tailor it according to expected load on the server.

[hamsterksu]

@hrosa do we need to have pool for AudioStreamCache? it can be a bottleneck too.
not sure that we need to use it. GC will remove extra objects.
also we don't create objects for each method call. we still use contains

[hrosa]

@hamsterksu I actually didn't notice that contains() there. Sorry about that.

Thinking about it, I believe we have potential for null pointers as is. If map.contains() returns true while another thread is removing that object from map, then this line will result in NPE.

A possible workaround is to use map.get(key):

Object obj = map.get(key);
if(obj == null) then
obj=map.putifabsent(key, new Object());

I agree that this way, pools shouldn't be required. Some trash can still be generated, but not for every request and I'm confident G1 can handle that.

Wdyt?

[hrosa]

Hi @hamsterksu @achikin

I believe I found one issue, first call (for a new audio file that is not yet cached) always fails.

The logs show that the audio file cannot be found, causing the Play signal to abort. An MGCP NFTY is sent immediately back to RestComm, so no audio can be heard.

11:33:19,092 INFO [Transaction] tx=147483653 Started, message= CRCX mobicents/bridge/$@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,096 INFO [Transaction] tx=147483653 was executed normaly
11:33:19,106 INFO [Transaction] tx=147483654 Started, message= MDCX mobicents/ivr/1@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,106 INFO [Transaction] tx=147483654 was executed normaly
11:33:19,117 INFO [Transaction] tx=147483655 Started, message= CRCX mobicents/bridge/1@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,136 INFO [Transaction] tx=147483655 was executed normaly
11:33:19,187 INFO [Transaction] tx=147483656 Started, message= RQNT mobicents/ivr/1@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,194 INFO Play Start announcement (segment=0)
11:33:19,199 INFO [Play] URL cannot be found: http://localhost:8080/restcomm/cache/ACae6e420f425248d6a26948c17a9e2acf/35ea210b73dcaa203f471c0e30304811a8b89b94a598aa9c44f1c3a5d8a7ce88.wav
11:33:19,200 INFO [Transaction] tx=2 Started, message= NTFY mobicents/ivr/1@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,200 INFO [Transaction] tx=147483656 was executed normaly
11:33:19,200 INFO [Transaction] tx=2 was executed normaly
11:33:19,206 WARN [Controller] Could not find MGCP transaction id=2
11:33:19,213 INFO [Transaction] tx=147483658 Started, message= DLCX mobicents/bridge/1@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,215 INFO [Transaction] tx=147483659 Started, message= DLCX mobicents/ivr/1@127.0.0.1:2427, call agent = localhost/127.0.0.1:2727
11:33:19,216 INFO [Transaction] tx=147483659 was executed normaly
11:33:19,219 INFO [Transaction] tx=147483658 was executed normaly

Can you verify please?

[hamsterksu]

@hrosa sure i will check it,

do you use separate instances of restcomm and mediaserver?

[hrosa]

@hamsterksu was testing locally, so RestComm and MS were running in same instance.

[hamsterksu]

hi @hrosa i have found issue
stream = cache.putIfAbsent(uri, new AudioStreamCache(uri));

putIfAbsent returns null if no such mapping existed http://www.ehcache.org/apidocs/3.1.1/org/ehcache/Cache.html#putIfAbsent-K-V-

unfortunately i removed prev fork so i have created new, applied pull request as patch and made fix
please find new PR here - #252