Skip to content

Conversation

jekh
Copy link
Collaborator

@jekh jekh commented Jan 31, 2015

This PR (unfortunately) removes the very good ExpirableMap class @nite23 created as part of PR #118. The reason for this is that I began encountering intermittent unit test failures in ExpirableMapTest when running on Java 8. The Java 8 implementation of ConcurrentHashMap has changed, apparently in a way that was occasionally breaking ExpirableMapTest.

I began to look into the issue to fix it, when I discovered that we were already including Guava as part of sitebricks, and on top of that, littleproxy includes Guava -- so we may as well leverage the expiring cache capabilities of Guava and save some additional maintenance.

In additional to replacing ExpirableMap, this PR also adds some unit tests to verify actual expiration (removal) of the expired proxy from the ProxyManager.

Important note: the guava cache does not proactively expire objects. Expiration checks happen on writes and "occasional" reads. This means there may be some overhead to other threads requesting new/deleting old proxies if there are expired proxies that need to be stopped.

It also means if you spin up a large number of proxies and then don't access them via REST calls, they "may" stay in memory past their TTL, perhaps indefinitely. I believe this is a sufficiently rare use case (typically clients would access the proxies they create, otherwise why create them?) so it should not be a noticeable problem. However, if others would prefer proactive expiration, this can be done easily by using the guava cache's cleanUp() method from a scheduled thread.

@jekh jekh force-pushed the replace-expirable-map-with-guava-cache branch 3 times, most recently from a90e428 to 0a41a74 Compare January 31, 2015 08:50
@jekh jekh force-pushed the replace-expirable-map-with-guava-cache branch 3 times, most recently from ec3f6d3 to 0a23ee6 Compare February 4, 2015 08:06
@jekh
Copy link
Collaborator Author

jekh commented Feb 4, 2015

After thinking about the expired proxy cleanup for a while, I think it makes sense to have a cleanup thread, as we did before. Clients could "forget" about proxies for any number of reasons that could leave the proxy manager idle, so I think it makes sense to have the proxy manager actively expire the proxies. I've added a scheduled daemon thread with a 60s delay to clean up expired entries from the proxy cache. Of course, any cache access can still trigger a cleanup, as before.

I'll leave this PR open for a few more days for comments before merging it in.

@jekh jekh force-pushed the replace-expirable-map-with-guava-cache branch from 0a23ee6 to ccbd337 Compare February 4, 2015 08:10
jekh added 2 commits February 4, 2015 16:40
…ecutor and cleanup thread for all ProxyManagers, and to prevent leaking ProxyManager instances
…-map-with-guava-cache

Conflicts:
	browsermob-rest/src/main/java/net/lightbody/bmp/proxy/ProxyManager.java
jekh added a commit that referenced this pull request Feb 8, 2015
@jekh jekh merged commit 8913c15 into lightbody:master Feb 8, 2015
@jekh jekh deleted the replace-expirable-map-with-guava-cache branch February 8, 2015 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant