Clean out all STs immediately when TGT is destroyed #1189
Conversation
| @@ -328,7 +316,8 @@ public ServiceTicket grantServiceTicket( | |||
| this.serviceTicketExpirationPolicy, | |||
| currentAuthentication != null); | |||
|
|
|||
| this.serviceTicketRegistry.addTicket(serviceTicket); | |||
| this.ticketRegistry.updateTicket(ticketGrantingTicket); | |||
There was a problem hiding this comment.
Indeed, if we have a new ST, the TGT must be updated, but I thought it was internally done by the AbstractDistributedRegistry by using the delegators: https://github.com/Jasig/cas/blob/master/cas-server-core/src/main/java/org/jasig/cas/ticket/registry/AbstractDistributedTicketRegistry.java#L230. Am I missing the point?
There was a problem hiding this comment.
You are right, I have missed this. The problem was that hazelcast registry which I was testing wasn't returning proxied ticket instance from getTicket method. Fixed this and rolled back changes related to addition of updateTicket:
mduszyk@3496d4f#diff-8f60343d0dc3c9f1ea4d75ddcfef314fR127
| @@ -384,7 +372,7 @@ public ServiceTicket grantServiceTicket(final String ticketGrantingTicketId, | |||
| public TicketGrantingTicket delegateTicketGrantingTicket(final String serviceTicketId, final Credential... credentials) | |||
| throws AuthenticationException, TicketException { | |||
|
|
|||
| final ServiceTicket serviceTicket = this.serviceTicketRegistry.getTicket(serviceTicketId, ServiceTicket.class); | |||
| final ServiceTicket serviceTicket = this.ticketRegistry.getTicket(serviceTicketId, ServiceTicket.class); | |||
There was a problem hiding this comment.
OK. So the idea here is to have only one tickets registry.
There was a problem hiding this comment.
Yes. The separate registry for STs was only added as part of a JBoss ticket registry when I traced the feature, and we no longer support that option, and I have never ever seen this separate registry used anywhere. Safe bet to remove it.
|
This is looking very good. Thank you. We do seem to have a build failure here, but I am inclined to blame it on Travis. Restarted the build, so we'll see what happens :) @mduszyk Have you by the way, tested this changeset under load in a HA environment? Would be great to have some real test results. |
|
Haven't tested it yet in HA env, only local tests on dev machine. I am going to push this to double instance CAS env with hazelcast cache as first stage of tests. |
|
Very good. I think we'd want to leave the change open here until we have some solid results from you or others. Since it's a substantial change to the ticket data model, would be great to confirm the behavior under load. |
| } | ||
| ticket.markTicketExpired(); | ||
|
|
||
| final Map<String, Service> services = ticket.getServices(); |
There was a problem hiding this comment.
I also think this deserves a comment, to explain why sync block is removed.
There was a problem hiding this comment.
Minor issue though. No biggie :)
There was a problem hiding this comment.
Sure, I removed this because this only deletes service tickets on local copy of TGT which has effect on registry only in case of memory registry (direct modification of the object kept in registry map). Second, this sync block works as intended only in case of memory registry because for cache based registry the TGT object pulled from registry will be diff object each time getTicket is called. Cache based registry will pull serialized data from cache and then build new object - this behaviour may be diff depending on actual cache used but I think it is safest to assume this scenario.
There was a problem hiding this comment.
The similar locking issue is here:
https://github.com/Jasig/cas/blob/4.1.x/cas-server-core/src/main/java/org/jasig/cas/CentralAuthenticationServiceImpl.java#L437
But it seems to be separate topic and it does no harm in my opinion, except this locking doesn't serialize any access in case of cache based registries.
There was a problem hiding this comment.
Appreciate the comment. Would you mind turning this into a Java comment, or better yet, push a commit that turns that into a javadoc?
There was a problem hiding this comment.
Added javadoc for 2 methods I found, which synchronize on tickets pulled from registry.
|
@leleuj you OK with the changeset? |
|
I'll be ok when the load testing will prove that performance are not compromised by the change (I'm thinking of the children tickets cleaning). |
|
Great. So @mduszyk the sooner you can post results, the better. This is because we have other pending pulls that are going to affect this changeset here, and we would like to finalize this before moving on. otherwise, it will be more work for you start merging with master later. Thanks again. |
|
I tested this in HA env, 2 CAS servers using hazelcast registry. Simulated 100 login sessions each created ST ticket and validated on both servers. Then each session performed logout and validated again after logout expecting failure. It worked fine for each session. |
|
Sounds good news! Did you already performed benchmarks without the change? What were the maximum you reach? |
|
Could you actually post the results? As in, how did you simulate logins? which tools? which parameters? what sort of environment? etc. |
|
I found out that some cache-based registries support bulk updates and removals. So potentially performance would be improved. Also, Even if we weren't removing all STs when TGT goes away, the cache at some point would be doing the same exact thing at a later interval, so whatever perf we have today remains the same with this change. |
|
P.S: We can work on bulk operations later for those caches that support it. |
|
The test was python script which was doing following: 2 Generate ST, (this step was repeated when more ST was generated) 3 Validate ST ticket - expect success 4 Logout (this request's time was measured) 5 Try to generate ST - expect failure 6 Try to validate ST generated in step 2 - expect failure: It was run multiple times (multiple processes spawned: 20, 50, 100, 200) each step here repeated 3 times. So 3 times 20 processes, 3 times 50 processes etc. All those tests were run against:
CAS instances were run on 2 vms which are part of the test env for the project I am working on. Both CAS were configured to use hazelcast registry and were talking to postgres db which is creds store. The test env wasn't isolated and there were possible other requests to CAS servers and DB during the test, so following results may not be very accurate. Here are results: Orig, 1 ST
Fixed, 1 ST
Fixed, 5 ST
|
|
Cool. Those are fairly excellent results. @leleuj do you concur? |
|
Yes, the performance is not as good as currently, but the degradation is fairly limited. A big thanks to @mduszyk for his tenacity on this. 👍 |
Clean out all STs immediately when TGT is destroyed
Clean out all STs immediately when TGT is destroyed
Closes https://github.com/Jasig/cas/issues/1183
Changes: