Looks like it is code review time - and you are catching a fair bit.
You are probably correct about the use of hashmap; I am always worried
to introduce a synchronized map with out looking at each place it is
used (for fear of deadlock).
Did you want to try patching that and seeing if it fixes the problem?
With respect to the use of ContentEntry.state - I had expected each
ContentEntry.state to be cleaned up when the FeatureStores using it
had been disposed.
However that would not work either since the Transaction is what makes
a ContentEntry useful; so we would need to clean up when the
transaction is no longer in use.
This is one to take to geotools-devel; and possibly two bug reports
since you have identified two problems?
On Sun, Nov 14, 2010 at 6:40 AM, Dustin Parker <dparker@...> wrote:
> Hello all, two questions/observations. First, a potential threading problem:
> I was looking at a running GeoServer (2.0.1, which means GeoTools 2.6.1)
> that had lots of transactions running against it and it started throwing
> NPEs in org.geotools.data.store.ContentEntry:131. I looked at that line:
> 130 ContentState auto = state.get(Transaction.AUTO_COMMIT);
> 131 ContentState copy = (ContentState) auto.copy(); // NPE
> 132 copy.setTransaction(transaction != null ? transaction :
> 133 state.put(transaction, copy);
> So it implies that 'auto' is null. However, the constructor says:
> 87 ContentState autoState = dataStore.createContentState(this);
> 88 autoState.setTransaction(Transaction.AUTO_COMMIT);
> 89 this.state.put(Transaction.AUTO_COMMIT, autoState);
> this.state.put is only called in two places: line 133 and line 89. Line 89
> happens first, associating Transaction.AUTO_COMMIT with a newly-created (and
> dereferenced, so non-null) ContentState. 133 associates a given transaction,
> transaction, with the ContentState associated with Transaction.AUTO_COMMIT,
> which we already must be non-null. So there is no chance of autoState ever
> being null, but it clearly was in the live system. I did a heap dump and ran
> jhat (I still have the heap dump, so I can answer any questions you may have
> about it, but I can't post it) and saw that there were instances of
> ContentEntry whose state map had no association for Transaction.AUTO_COMMIT!
> This should have been impossible, so it leaves only the possibility of map
> corruption by unsynchronized access (ContentEntry.state is a plain HashMap).
> If this really is a problem, it would explain some of the random failures
> we've been having....
> Second, a potential memory leak:
> Still looking at the same class in jhat, I noticed some of the maps had >
> 6,000 entries, which doesn't make sense (we don't do THAT much concurrent
> access; we try to keep to one transaction at a time per feature type).
> Looking over the code again, it seems that ContentEntry.state is added to a
> lot, but its entries are never cleaned up anywhere. I looked high and low,
> but I couldn't find where the end of a transaction causes its state to be
> cleaned from ContentEntry. The only other possibility is that instances of
> ContentEntry themselves are recycled and recreated every now and then, but I
> couldn't find that either (ContentEntry.dispose() is only called when the
> data store itself is being disposed). Did I really find a problem or am I
> missing something obvious?
> Thank you all,
> Dustin Parker - Forward Slope, Inc.
> Cell: 619 277 2591
> Centralized Desktop Delivery: Dell and VMware Reference Architecture
> Simplifying enterprise desktop deployment and management using
> Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
> client virtualization framework. Read more!
> Geotools-gt2-users mailing list