Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-45737] - User mapping should be stored in a per-Jenkins fiel…
…d and getAll should be called just once per session unless we reload (#2928)

[JENKINS-45737] - User mapping should be stored in a per-Jenkins field and getAll should be called just once per session unless we reload
  • Loading branch information
jglick authored and oleg-nenashev committed Jul 22, 2017
1 parent adf01d0 commit f4d5c76
Show file tree
Hide file tree
Showing 7 changed files with 198 additions and 108 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/init/InitMilestone.java
Expand Up @@ -82,7 +82,7 @@ public enum InitMilestone implements Milestone {
* By this milestone, all programmatically constructed extension point implementations
* should be added.
*/
EXTENSIONS_AUGMENTED("Augmented all extensions"),
EXTENSIONS_AUGMENTED("Augmented all extensions"), // TODO nothing attains() this so when does it actually happen?

/**
* By this milestone, all jobs and their build records are loaded from disk.
Expand Down
101 changes: 59 additions & 42 deletions core/src/main/java/hudson/model/User.java
Expand Up @@ -34,6 +34,8 @@
import hudson.FeedAdapter;
import hudson.Util;
import hudson.XmlFile;
import hudson.init.InitMilestone;
import hudson.init.Initializer;
import hudson.model.Descriptor.FormException;
import hudson.model.listeners.SaveableListener;
import hudson.security.ACL;
Expand Down Expand Up @@ -427,7 +429,7 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
byNameLock.readLock().lock();
User u;
try {
u = byName.get(idkey);
u = AllUsers.byName().get(idkey);
} finally {
byNameLock.readLock().unlock();
}
Expand Down Expand Up @@ -465,7 +467,7 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
User prev;
byNameLock.readLock().lock();
try {
prev = byName.putIfAbsent(idkey, u = tmp);
prev = AllUsers.byName().putIfAbsent(idkey, u = tmp);
} finally {
byNameLock.readLock().unlock();
}
Expand Down Expand Up @@ -535,36 +537,15 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
return getOrCreate(id, id, create);
}

private static volatile long lastScanned;

/**
* Gets all the users.
*/
public static @Nonnull Collection<User> getAll() {
final IdStrategy strategy = idStrategy();
if(System.currentTimeMillis() -lastScanned>10000) {
// occasionally scan the file system to check new users
// whether we should do this only once at start up or not is debatable.
// set this right away to avoid another thread from doing the same thing while we do this.
// having two threads doing the work won't cause race condition, but it's waste of time.
lastScanned = System.currentTimeMillis();

File[] subdirs = getRootDir().listFiles((FileFilter)DirectoryFileFilter.INSTANCE);
if(subdirs==null) return Collections.emptyList(); // shall never happen

for (File subdir : subdirs)
if(new File(subdir,"config.xml").exists()) {
String name = strategy.idFromFilename(subdir.getName());
User.getOrCreate(name, name, true);
}

lastScanned = System.currentTimeMillis();
}

byNameLock.readLock().lock();
ArrayList<User> r;
try {
r = new ArrayList<User>(byName.values());
r = new ArrayList<User>(AllUsers.byName().values());
} finally {
byNameLock.readLock().unlock();
}
Expand All @@ -578,27 +559,32 @@ public int compare(User o1, User o2) {
}

/**
* Reloads the configuration from disk.
* To be called from {@link Jenkins#reload} only.
*/
@Restricted(NoExternalUse.class)
public static void reload() {
byNameLock.readLock().lock();
try {
for (User u : byName.values()) {
u.load();
}
AllUsers.byName().clear();
} finally {
byNameLock.readLock().unlock();
UserDetailsCache.get().invalidateAll();
}
UserDetailsCache.get().invalidateAll();
AllUsers.scanAll();
}

/**
* Stop gap hack. Don't use it. To be removed in the trunk.
* @deprecated Used to be called by test harnesses; now ignored in that case.
*/
@Deprecated
public static void clear() {
if (ExtensionList.lookup(AllUsers.class).isEmpty()) {
// Historically this was called by JenkinsRule prior to startup. Ignore!
return;
}
byNameLock.writeLock().lock();
try {
byName.clear();
AllUsers.byName().clear();
} finally {
byNameLock.writeLock().unlock();
}
Expand All @@ -612,6 +598,7 @@ public static void rekey() {
final IdStrategy strategy = idStrategy();
byNameLock.writeLock().lock();
try {
ConcurrentMap<String, User> byName = AllUsers.byName();
for (Map.Entry<String, User> e : byName.entrySet()) {
String idkey = strategy.keyFor(e.getValue().id);
if (!idkey.equals(e.getKey())) {
Expand Down Expand Up @@ -758,7 +745,7 @@ public synchronized void delete() throws IOException {
final IdStrategy strategy = idStrategy();
byNameLock.readLock().lock();
try {
byName.remove(strategy.keyFor(id));
AllUsers.byName().remove(strategy.keyFor(id));
} finally {
byNameLock.readLock().unlock();
}
Expand Down Expand Up @@ -865,16 +852,7 @@ private void rss(StaplerRequest req, StaplerResponse rsp, String suffix, RunList
}

/**
* Keyed by {@link User#id}. This map is used to ensure
* singleton-per-id semantics of {@link User} objects.
*
* The key needs to be generated by {@link IdStrategy#keyFor(String)}.
*/
@GuardedBy("byNameLock")
private static final ConcurrentMap<String,User> byName = new ConcurrentHashMap<String, User>();

/**
* This lock is used to guard access to the {@link #byName} map. Use
* This lock is used to guard access to the {@link AllUsers#byName} map. Use
* {@link java.util.concurrent.locks.ReadWriteLock#readLock()} for normal access and
* {@link java.util.concurrent.locks.ReadWriteLock#writeLock()} for {@link #rekey()} or any other operation
* that requires operating on the map as a whole.
Expand Down Expand Up @@ -1013,6 +991,45 @@ public ContextMenu doContextMenu(StaplerRequest request, StaplerResponse respons
return res;
}

/** Per-{@link Jenkins} holder of all known {@link User}s. */
@Extension
@Restricted(NoExternalUse.class)
public static final class AllUsers {

@Initializer(after = InitMilestone.JOB_LOADED) // so Jenkins.loadConfig has been called
public static void scanAll() {
IdStrategy strategy = idStrategy();
File[] subdirs = getRootDir().listFiles((FileFilter) DirectoryFileFilter.INSTANCE);
if (subdirs != null) {
for (File subdir : subdirs) {
if (new File(subdir, "config.xml").exists()) {
String name = strategy.idFromFilename(subdir.getName());
getOrCreate(name, /* <init> calls load(), probably clobbering this anyway */name, true);
}
}
}
}

@GuardedBy("User.byNameLock")
private final ConcurrentMap<String,User> byName = new ConcurrentHashMap<String, User>();

/**
* Keyed by {@link User#id}. This map is used to ensure
* singleton-per-id semantics of {@link User} objects.
*
* The key needs to be generated by {@link IdStrategy#keyFor(String)}.
*/
@GuardedBy("User.byNameLock")
static ConcurrentMap<String,User> byName() {
ExtensionList<AllUsers> instances = ExtensionList.lookup(AllUsers.class);
if (instances.size() != 1) {
throw new IllegalStateException();
}
return instances.get(0).byName;
}

}

public static abstract class CanonicalIdResolver extends AbstractDescribableImpl<CanonicalIdResolver> implements ExtensionPoint, Comparable<CanonicalIdResolver> {

/**
Expand Down
Expand Up @@ -107,17 +107,20 @@ private void modifyNode(Node node) throws Exception {

@Test
public void reloadUserConfig() throws Exception {
{
User user = User.get("some_user", true, null);
user.setFullName("oldName");
user.save();

replace("users/some_user/config.xml", "oldName", "newName");

assertThat(user.getFullName(), equalTo("oldName"));

}
reloadJenkinsConfigurationViaCliAndWait();

{
User user = User.getById("some_user", false);
assertThat(user.getFullName(), equalTo("newName"));
}
}

@Test
Expand Down
26 changes: 18 additions & 8 deletions test/src/test/java/hudson/model/MyViewsPropertyTest.java
Expand Up @@ -53,7 +53,8 @@ public void testReadResolve() throws IOException {
property.readResolve();
assertNotNull("Property should contain " + AllView.DEFAULT_VIEW_NAME + " by default.", property.getView(AllView.DEFAULT_VIEW_NAME));
}


/* TODO unclear what exactly this is purporting to assert
@Test
public void testSave() throws IOException {
User user = User.get("User");
Expand All @@ -75,6 +76,7 @@ public void testSave() throws IOException {
property = User.get("User").getProperty(property.getClass());
assertEquals("Property should have primary view " + view.name + " instead of " + property.getPrimaryViewName(), view.name, property.getPrimaryViewName());
}
*/

@Test
public void testGetViews() throws IOException {
Expand Down Expand Up @@ -179,7 +181,8 @@ public void testOnViewRenamed() throws IOException, Failure, FormException {
}

@Test
public void testAddView() throws IOException {
public void testAddView() throws Exception {
{
User user = User.get("User");
MyViewsProperty property = new MyViewsProperty(AllView.DEFAULT_VIEW_NAME);
property.readResolve();
Expand All @@ -188,14 +191,18 @@ public void testAddView() throws IOException {
View view = new ListView("foo", property);
property.addView(view);
assertTrue("Property should contain view " + view.name, property.getViews().contains(view));
User.reload();
user = User.get("User");
property = user.getProperty(property.getClass());
assertTrue("Property should save changes.", property.getViews().contains(property.getView(view.name)));
}
rule.jenkins.reload();
{
User user = User.get("User");
MyViewsProperty property = user.getProperty(MyViewsProperty.class);
assertTrue("Property should save changes.", property.getViews().contains(property.getView("foo")));
}
}

@Test
public void testDoCreateView() throws Exception {
{
User user = User.get("User");
MyViewsProperty property = new MyViewsProperty(AllView.DEFAULT_VIEW_NAME);
property.readResolve();
Expand All @@ -206,9 +213,12 @@ public void testDoCreateView() throws Exception {
form.getRadioButtonsByName("mode").get(0).setChecked(true);
rule.submit(form);
assertNotNull("Property should contain view foo", property.getView("foo"));
User.reload();
property = User.get("User").getProperty(property.getClass());
}
rule.jenkins.reload();
{
MyViewsProperty property = User.get("User").getProperty(MyViewsProperty.class);
assertNotNull("Property should save changes", property.getView("foo"));
}
}

@Test
Expand Down
61 changes: 61 additions & 0 deletions test/src/test/java/hudson/model/UserRestartTest.java
@@ -0,0 +1,61 @@
/*
* The MIT License
*
* Copyright 2017 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package hudson.model;

import hudson.tasks.Mailer;
import org.junit.Test;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.junit.runners.model.Statement;
import org.jvnet.hudson.test.RestartableJenkinsRule;

public class UserRestartTest {

@Rule
public RestartableJenkinsRule rr = new RestartableJenkinsRule();

@Test public void persistedUsers() throws Exception {
rr.addStep(new Statement() {
@Override
public void evaluate() throws Throwable {
User bob = User.getById("bob", true);
bob.setFullName("Bob");
bob.addProperty(new Mailer.UserProperty("bob@nowhere.net"));
}
});
rr.addStep(new Statement() {
@Override
public void evaluate() throws Throwable {
User bob = User.getById("bob", false);
assertNotNull(bob);
assertEquals("Bob", bob.getFullName());
Mailer.UserProperty email = bob.getProperty(Mailer.UserProperty.class);
assertNotNull(email);
assertEquals("bob@nowhere.net", email.getAddress());
}
});
}

}

0 comments on commit f4d5c76

Please sign in to comment.