ahubold commented on a change in pull request #2049:
URL: https://github.com/apache/lucene-solr/pull/2049#discussion_r514931450
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1253,77 +1254,90 @@ public SolrCore create(String coreName, Map<String,
String> parameters) {
* @return the newly created core
*/
public SolrCore create(String coreName, Path instancePath, Map<String,
String> parameters, boolean newCollection) {
-
- CoreDescriptor cd = new CoreDescriptor(coreName, instancePath, parameters,
getContainerProperties(), getZkController());
-
- // TODO: There's a race here, isn't there?
- // Since the core descriptor is removed when a core is unloaded, it should
never be anywhere when a core is created.
- if (getAllCoreNames().contains(coreName)) {
- log.warn("Creating a core with existing name is not allowed");
- // TODO: Shouldn't this be a BAD_REQUEST?
- throw new SolrException(ErrorCode.SERVER_ERROR, "Core with name '" +
coreName + "' already exists.");
- }
-
- // Validate paths are relative to known locations to avoid path traversal
- assertPathAllowed(cd.getInstanceDir());
- assertPathAllowed(Paths.get(cd.getDataDir()));
-
- boolean preExisitingZkEntry = false;
try {
- if (getZkController() != null) {
- if (cd.getCloudDescriptor().getCoreNodeName() == null) {
- throw new SolrException(ErrorCode.SERVER_ERROR, "coreNodeName
missing " + parameters.toString());
+ synchronized (inFlightCreations) {
+ if (inFlightCreations.contains(coreName)) {
+ String msg = "Already creating a core with name '" + coreName + "',
call aborted '";
+ log.warn(msg);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg);
}
- preExisitingZkEntry =
getZkController().checkIfCoreNodeNameAlreadyExists(cd);
+ inFlightCreations.add(coreName);
+ }
+ CoreDescriptor cd = new CoreDescriptor(coreName, instancePath,
parameters, getContainerProperties(), getZkController());
+
+ // TODO: There's a race here, isn't there?
+ // Since the core descriptor is removed when a core is unloaded, it
should never be anywhere when a core is created.
+ if (getAllCoreNames().contains(coreName)) {
+ log.warn("Creating a core with existing name is not allowed");
+ // TODO: Shouldn't this be a BAD_REQUEST?
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Core with name '" +
coreName + "' already exists.");
}
- // Much of the logic in core handling pre-supposes that the
core.properties file already exists, so create it
- // first and clean it up if there's an error.
- coresLocator.create(this, cd);
+ // Validate paths are relative to known locations to avoid path traversal
+ assertPathAllowed(cd.getInstanceDir());
+ assertPathAllowed(Paths.get(cd.getDataDir()));
- SolrCore core = null;
+ boolean preExisitingZkEntry = false;
try {
- solrCores.waitAddPendingCoreOps(cd.getName());
- core = createFromDescriptor(cd, true, newCollection);
- coresLocator.persist(this, cd); // Write out the current core
properties in case anything changed when the core was created
- } finally {
- solrCores.removeFromPendingOps(cd.getName());
- }
+ if (getZkController() != null) {
+ if (cd.getCloudDescriptor().getCoreNodeName() == null) {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "coreNodeName
missing " + parameters.toString());
+ }
+ preExisitingZkEntry =
getZkController().checkIfCoreNodeNameAlreadyExists(cd);
+ }
- return core;
- } catch (Exception ex) {
- // First clean up any core descriptor, there should never be an existing
core.properties file for any core that
- // failed to be created on-the-fly.
- coresLocator.delete(this, cd);
- if (isZooKeeperAware() && !preExisitingZkEntry) {
+ // Much of the logic in core handling pre-supposes that the
core.properties file already exists, so create it
+ // first and clean it up if there's an error.
+ coresLocator.create(this, cd);
+
+ SolrCore core = null;
try {
- getZkController().unregister(coreName, cd);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- SolrException.log(log, null, e);
- } catch (KeeperException e) {
- SolrException.log(log, null, e);
- } catch (Exception e) {
- SolrException.log(log, null, e);
+ solrCores.waitAddPendingCoreOps(cd.getName());
+ core = createFromDescriptor(cd, true, newCollection);
+ coresLocator.persist(this, cd); // Write out the current core
properties in case anything changed when the core was created
+ } finally {
+ solrCores.removeFromPendingOps(cd.getName());
}
- }
- Throwable tc = ex;
- Throwable c = null;
- do {
- tc = tc.getCause();
- if (tc != null) {
- c = tc;
+ return core;
+ } catch (Exception ex) {
+ // First clean up any core descriptor, there should never be an
existing core.properties file for any core that
+ // failed to be created on-the-fly.
+ coresLocator.delete(this, cd);
+ if (isZooKeeperAware() && !preExisitingZkEntry) {
+ try {
+ getZkController().unregister(coreName, cd);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ SolrException.log(log, null, e);
+ } catch (KeeperException e) {
+ SolrException.log(log, null, e);
+ } catch (Exception e) {
+ SolrException.log(log, null, e);
+ }
}
- } while (tc != null);
- String rootMsg = "";
- if (c != null) {
- rootMsg = " Caused by: " + c.getMessage();
- }
+ Throwable tc = ex;
+ Throwable c = null;
+ do {
+ tc = tc.getCause();
+ if (tc != null) {
+ c = tc;
+ }
+ } while (tc != null);
+
+ String rootMsg = "";
+ if (c != null) {
+ rootMsg = " Caused by: " + c.getMessage();
+ }
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
- "Error CREATEing SolrCore '" + coreName + "': " + ex.getMessage() +
rootMsg, ex);
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+ "Error CREATEing SolrCore '" + coreName + "': " + ex.getMessage()
+ rootMsg, ex);
+ }
+ } finally {
+ synchronized (inFlightCreations) {
+ inFlightCreations.remove(coreName);
Review comment:
There's still an error here. The finally block removes the coreName from
inFlightCreations even if it was added by a previous call, i.e. even if this
call throws a SolrException because the coreName was already contained in
inFlightCreations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
