[JENKINS-54900] improve concurrent security

[runzexia]

In my usage scenario, I will insert a lot of roles. The order of insertion is slow for me.When I try to add a role concurrently, I get an exception.
So I tried to use a concurrent safe container to store the role.And I packaged the plugin and tested the concurrent inserts in my environment.No exceptions continue to appear, and everything seems to be normal.
Although replacing the container does not solve all the concurrency problems, I think it is effective for concurrent insertion of non-duplicate data.

[oleg-nenashev]

It won't work for roles which are already persisted on the disk.
Also it rather works around the problem than fixes it. XStream persists the object, and it won't care about the object's state. So it may still be serialized incorrectly.

My advice would be to override writeObject() to hook on the serialization logic, and then to synchronize access using synchronized

[runzexia]

override writeObject()

[oleg-nenashev]

It does not solve the comment.

1. Access to grantedRoles needs to be synchronized, and it's not
2. Sync of access to the writeObject in RoleStrategy actually does not do what is expected, RoleMap API methods are still not synced

@runzexia I wish I could be able to write a patch as I see it, but I do not have enough time this week. If you need more explanation, maybe it's better to discuss the approach in the dev mailing list

[runzexia]

@oleg-nenashev Thank you, I will start a discussion on the dev mailing list.

[runzexia]

I asked in the dev mail list but I haven't replied yet, so I modified the PR according to my own thoughts. If there is time, can I help me review it?

1. Replace the thread-unsafe data structure so that the authentication operation can still be performed when the modification is made.
2. Use synchronized for thread synchronization for all write operations
3. And rewrite the writeObject method to get the latest state of the object, to ensure that serialization is saved.

[oleg-nenashev]

If you add a ConcurrentHashMap, maybe there is no need to sync all access operations. The same could be done by adding a readResolve() logic with data migration, I suspect

Maybe it's also better to switch to https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/util/CopyOnWriteMap.java to optimize the performance without manual locks. Write operations here are rare, so it should be effective enough and tolerant against JEP-200 risks

[runzexia]

I think rewriting readResolve() does not solve this problem, the problem occurs in the concurrency of write operations and iterative operations. Iterative operations are also performed during authentication. So replacing the data type should fix it

[runzexia]

Hi @oleg-nenashev , do you think the current method can solve this problem? If you can solve it, merge the code?

[oleg-nenashev]

AFAICT replacing the data type fixes the issue only for new roles. I will try to review it and to amend the patch this weekend (if needed). Anyway, it is a time for a new release

[oleg-nenashev]

I did some testing, and actually it is going to work well as is, thanks to the legacy deserialization logic

[oleg-nenashev]

Thanks @runzexia !