Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

Rest memory issues to address #765 #1374

Merged
merged 2 commits into from
Aug 29, 2014

Conversation

digitaldan
Copy link
Contributor

This upgrades the Atmosphere version to 2.0.9, it also changes the way we use the UUIDBroadcaster class so that the proper life cycle management (configure, start, stop) is utilized. There was a comment in the code that the CV rest io bundle may interfere with how we are now telling Atmosphere to use the UUIDBroadcaster class, I did not see any issues, but I only enabled the CV bundle, I did not try using it.

@digitaldan
Copy link
Contributor Author

This is in reference to #765

@buildhive
Copy link

openhab » openhab #1120 SUCCESS
This pull request looks good
(what's this?)

@teichsta teichsta added the bug label Aug 29, 2014
@teichsta teichsta added this to the 1.5.1 milestone Aug 29, 2014
teichsta added a commit that referenced this pull request Aug 29, 2014
@teichsta teichsta merged commit 9435ad4 into openhab:master Aug 29, 2014
@peuter
Copy link
Member

peuter commented Aug 29, 2014

This breaks the CV-bundle (or the REST bundle). Depending on the order both bundles are loaded the last one´s BroadcasterCache is used for both bundles. As long as you don´t use the CV bundle and the rest bundle is loaded later you experience no problems, as you already noticed.

Isn´t it possible to just configure + start the cache manually by:

broadcaster.getBroadcasterConfig().setBroadcasterCache(new UUIDBroadcasterCache()); broadcaster.getBroadcasterConfig().getBroadcasterCache().configure(broadcaster.getBroadcasterConfig());
broadcaster.getBroadcasterConfig().getBroadcasterCache().start();

or am I missing something?

@digitaldan
Copy link
Contributor Author

@peuter I will give that a try, I had initially tried starting it manually, but ran into a few NPE issues inside the atmosphere class, I will look at it again as I may have missed a configure step, and I don't honestly remember the specifics of what I tried. I am also hunting down another small leak in a static map in ResourceStateChangeListener that I want to fix as well.

@peuter
Copy link
Member

peuter commented Aug 29, 2014

@digitaldan I had NPE too, you have to call configure() before start() and they´re gone. A quick test looks good so far, the internal "garbage collection" where inactive clients and messages are removed from the cache is called regulary.

@digitaldan
Copy link
Contributor Author

Just made that change now and running some tests under load, thanks for the
tip.

On Fri, Aug 29, 2014 at 10:28 AM, peuter [email protected] wrote:

@digitaldan https://github.com/digitaldan I had NPE too, you have to
call configure() before start() and they´re gone. A quick test looks good
so far, the internal "garbage collection" where inactive clients and
messages are removed from the cache is called regulary.


Reply to this email directly or view it on GitHub
#1374 (comment).

@digitaldan digitaldan deleted the rest-memory-issues2 branch August 29, 2014 17:20
@digitaldan
Copy link
Contributor Author

#1376 incorporates @peuter tip. It should be safe to merge.

hubermi pushed a commit to hubermi/openhab that referenced this pull request Jan 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants