Fixes for issue-15

[msalle]

this pull-request should fix issue 15, adding two new PIPs with tests and test resources and the necessary configuration in the pepd.ini file. It also updates the junit version to 4.10 and fixes two warnings about deprecated methods (Element.getValue()). Furthermore, it adds two new .info files into /etc/grid-security/certificates, in order to have easier ways of describing policies by using a single name instead of 3 or 4 individual policy names.

[andreaceccanti]

Hi Mischa,

had to revert the changes in this PR, for the following reasons:

• it makes the build fail, since the new code expects the /etc/grid-security/certificates directory to be available on the build node (which is a requirement we don't want)
• the sync code in the PolicyNamesPIP IMO doesn't prevent concurrent updates to the cache. I suggest using a synchronized block or another reliable method for guarding against the concurrent update of the cache

[msalle]

Hi Andrea, the first was actually a bug in the code. It should ignore missing files, which it now does (see https://github.com/msalle/argus-pep-server/commit/87e5f37d75af719f4f277f21191d33f6e6302fe1). The other point should not be an issue, see https://github.com/msalle/argus-pep-server/blob/1_7/src/main/java/org/glite/authz/pep/pip/provider/PolicyNamesPIP.java#L296-L313 Perhaps not the most clean way of doing it, but I think it should be good enough? Or perhaps you're talking about some other point...?

[andreaceccanti]

The other point should not be an issue, see https://github.com/msalle/argus-pep-server/blob/1_7/src/main/java/org/glite/authz/pep/pip/provider/PolicyNamesPIP.java#L296-L313

That code is the issue! The updating field is not providing any locking guarantees. You should put the cache update in a synchronized section.

[msalle]

Could you elaborate on that? Setting a boolean should be an atomic operation, according to https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html being a primitive, not long or double. So why would that not work for a lock? I don't mind declaring the updateCache() as synchronized (that is what you suggest, right?) but I would like to understand why it is wrong what I'm doing...

[andreaceccanti]

That is precisely the point. AFAIU the change to the value is atomic, but not necessarily visible to another thread! Probably declaring the boolean volatile could be enough, but I think the responsibility of refreshing the cache is probably better implemented in the cache class itself, and transparent to the PIP.

[msalle]

Right, that makes more sense. I agree that volatile is indeed needed for the reading. The problem with a synchronized is that all other threads are probably blocking and that's not needed.

I agree it looks more logical to implement it somehow in the Cache class itself, but I don't really see how: What now happens is that the updateCache() method creates a new Cache instance when needed, using the old one as input, and then replace the old one with the new one once done. So from the point of view of Cache there is no updating, only an initialization. After that it's read-only. The PIP itself is the one that sees an updating and it's a very simple operation, which is why I left it as such.

If you have a more elegant solution, please let me know...

[andreaceccanti]

I suggest an approach similar to the one implemented in

https://github.com/italiangrid/voms-api-java/blob/master/src/main/java/org/italiangrid/voms/store/impl/DefaultVOMSTrustStore.java

which uses a ReadWriteLock:

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReadWriteLock.html https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html

[msalle]

I'm implementing currently a simple intermediate class, handling the updating and keeping track of the actual cache and being the only entry point for the PIP. This then hides the updating completely from the PIP itself. I would like to keep making sure that the reading is not blocked, so reading whilst updating is in progress should simply use the previous cache. As far as I can see, for that it should be sufficient to make the 'lock' variable and the 'cache' variable volatile. Do you agree with this?

[andreaceccanti]

Do you agree with this?

Hard to judge without seeing the code, but still I would not bother about implementing the sync by hand and would leverage the ReadWriteLock implementation, that is for sure correct.

[msalle]

I've just commited a new branch caching_update on my own fork, which is a code simplification and mostly move to a different class. Feel free to file a pull request which uses the ReadWriteLock, otherwise I will see if I can manage tomorrow.

[msalle]

Cleaned up the branches and rebased a new pull request w.r.t. your 1_7 branch. See new branch https://github.com/msalle/argus-pep-server/tree/new_pips_v2 and pull request https://github.com/argus-authz/argus-pep-server/pull/18