You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2023/01/18 22:11:28 UTC
[GitHub] [solr] magibney opened a new pull request, #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
magibney opened a new pull request, #1302:
URL: https://github.com/apache/solr/pull/1302
See: SOLR-16628
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1080640747
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
} catch (IOException e) {
throw new SolrException(ErrorCode.SERVER_ERROR, e);
+ } finally {
+ // according to spec, XML parser should close InputStream when parsing is complete.
+ // But in the event that this doesn't happen (either because an exception is
+ // thrown or because of an error in parser implementation, here we ensure that it
+ // is closed.
Review Comment:
BTW I appreciate code comments but I don't think you need to be so wordy to justify closing ourselves. IMO this can be summarized to one line:
> XML Parser *should* close but in exceptional cases might not; so let's be safe
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081919580
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -48,6 +50,7 @@
import org.xml.sax.InputSource;
/** Factory for ManagedIndexSchema */
+@NotThreadSafe
Review Comment:
@sonatype-lift ignore
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1405169779
With 9.1.1 released, added a CHANGES.txt entry for 9.2.0 release. Pending any feedback, I plan to merge this tomorrow.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney merged pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney merged PR #1302:
URL: https://github.com/apache/solr/pull/1302
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1397379968
I tried reproducing this by stressing my system, but haven't been able to, FYI.
Separately, is the value/risk ratio of this change enough to warrant inclusion in an RC3 for 9.1.1?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081460073
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
<picture><img alt="7% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/7/display.svg"></picture>
đŦ 3 similar findings have been found in this PR
---
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `ManagedIndexSchemaFactory.create(...)` reads without synchronization from `this.loadedResource`. Potentially races with write in method `ManagedIndexSchemaFactory.create(...)`.
Reporting because this access may occur on a background thread.
---
<details><summary><b>đ Expand here to view all instances of this finding</b></summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java | [207](https://github.com/apache/solr/blob/cd4923ff78ae43d80b2b43029ff282e5322a8e13/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java#L207) |
| solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java | [280](https://github.com/apache/solr/blob/cd4923ff78ae43d80b2b43029ff282e5322a8e13/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java#L280) |
| solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java | [208](https://github.com/apache/solr/blob/cd4923ff78ae43d80b2b43029ff282e5322a8e13/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java#L208) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GQ58R7X279C7SKRG4CCB9H9B?t=Infer|THREAD_SAFETY_VIOLATION" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=370237930&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=370237930&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=370237930&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=370237930&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=370237930&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081328045
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
Review Comment:
I think IndexSchemaFactory#loadConfig has the same pattern here? at least using `loader.openResource(name)` without closing it?
https://github.com/apache/solr/pull/987/files#diff-5965edf45e1dec49fea047168b81ddffbb530f5db8875b41627950773d63b3a6R138
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1090088022
##########
solr/core/build.gradle:
##########
@@ -115,6 +115,8 @@ dependencies {
implementation 'commons-cli:commons-cli'
+ implementation 'net.jcip:jcip-annotations'
Review Comment:
đ thanks for catching this, and for the work on #1313. I'll wait til after that's merged to merge this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1080690072
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
Review Comment:
I think we cannot, unfortunately; because in most cases the InputStream should have been closed properly by the parser before reaching our `finally` block, so we're really relying on the "exception-swallowing" quality of `IOUtils.closeQuietly()`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1397430768
Ok, so targeting 9.2 release. Does this warrant a CHANGES.txt entry? I'm thinking "no" ... nobody has actually reported this being a problem in the wild, afaik?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1080639405
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
Review Comment:
Can we use try-with-resources pattern here?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1089357566
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -290,8 +301,9 @@ public ManagedIndexSchema create(
return schema;
}
- private InputStream readSchemaLocally() {
+ private InputStream readSchemaLocally(String[] loadedResourceRef) {
Review Comment:
đ ... addressed this with 39c5620cd8f7ae96f98dde384b443ebe6d6ff318
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1099643465
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +200,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ Entry<String, InputStream> localSchemaInput = readSchemaLocally();
Review Comment:
<picture><img alt="5% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/5/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Unprotected write. Non-private method `ManagedIndexSchemaFactory.create(...)` indirectly writes to field `this.managedSchemaResourceName` outside of synchronization.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=384928906&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=384928906&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=384928906&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=384928906&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=384928906&lift_comment_rating=5)â]
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1090073203
##########
solr/core/build.gradle:
##########
@@ -115,6 +115,8 @@ dependencies {
implementation 'commons-cli:commons-cli'
+ implementation 'net.jcip:jcip-annotations'
Review Comment:
These two JARs are equivalent but have different licenses. We should prefer `stephenc` as it's ASL licensed; a clean-room written implementation. The net.jcip https://jcip.net/annotations/doc/index.html is using Creative Commons Attribution License (http://creativecommons.org/licenses/by/2.5) which the ASF is okay with in binary form https://www.apache.org/legal/resolved.html#cc-by but lets prefer the ASL one as we have a choice.
I removed net.jcip in #1313 this weekend; pending merge.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1419258816
@magibney #1313 was merged so this should be good to go resolving any conflicts :D
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1080647796
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
Review Comment:
<picture><img alt="15% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/15/display.svg"></picture>
*NULL_DEREFERENCE:* object returned by `getDocument(xml)` could be null and is dereferenced at line 403.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=369813981&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=369813981&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=369813981&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=369813981&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=369813981&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1082129194
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -48,6 +50,7 @@
import org.xml.sax.InputSource;
/** Factory for ManagedIndexSchema */
+@NotThreadSafe
Review Comment:
I recall this violation has come up before from other places that indirectly touch JSONParser. I don't know where it's happening here either but it doesn't matter. I'd say mark JSONParser.DevNull as ThreadSafe and Immutable but it's in SolrJ, thus introducing a dependency to JCIP. I'd rather another PR deal with that matter.
BTW you needn't tell Lift "ignore"; it's intended for projects that gate PR acceptance on this GitHub action. We gate on nothing but trust :-)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081887649
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -48,6 +50,7 @@
import org.xml.sax.InputSource;
/** Factory for ManagedIndexSchema */
+@NotThreadSafe
Review Comment:
<picture><img alt="7% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/7/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `ManagedIndexSchemaFactory.create(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=370425585&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=370425585&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=370425585&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=370425585&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=370425585&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081643380
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
A dev there clarified that NotThreadSafe should be understood! I think I'd prefer that to 3a047d4 which feels awkward because the Array wrapping for modifying loadedResource.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081414524
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
Review Comment:
I went ahead and used try-with-resources (b6487869eec05cad5478fea05fc8bf224ccb8129) where possible (it's not possible in contexts where the InputStream is opened in less straightforward ways).
@risdenk in cd4923ff78ae43d80b2b43029ff282e5322a8e13 I also addressed the case you identified in IndexSchemaFactory, and followed up the stack trace to address a similar issue in ManagedIndexSchemaFactory.
As a consequence of the ManagedIndexSchemaFactory change, this PR's a bit easier to review ignoring whitespace: https://github.com/apache/solr/pull/1302/files?w=1
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081648457
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
You're ok with 3a047d404b4e994865711229329a7461a751897b ? The array wrapping is admittedly awkward, but IMO much clearer than (and preferable to) setting an instance variable in one method and then checking its value in another, which is the status quo.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081568912
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1080763089
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
Review Comment:
I checked and I think this is wrong. It's a getter to a final field populated by methods that call a chain of methods terminating in DocumentBuilder.parse() that doesn't return null.
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
Review Comment:
Not sure if we *need* that. I was able to apply your change here to 9.1 in my reproducing setup. Your change, as well as a tweak to use the simpler try-with-resources both yielded working test runs.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1396382429
I found a reproducible line on branch_9_1 using JDK 11 on my aging laptop.
> gradlew -p solr/core test -Ptests.iters=10 --tests TestCoreContainer
Applying the changes here made this pass! Also with simpler try-with-resources pattern in one file.
Thanks again.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081669832
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +196,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ String[] loadedResourceRef = new String[1];
+ schemaInputStream = readSchemaLocally(loadedResourceRef);
Review Comment:
<picture><img alt="7% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/7/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `ManagedIndexSchemaFactory.create(...)` indirectly writes to field `this.managedSchemaResourceName` outside of synchronization.
Reporting because this access may occur on a background thread.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=370355097&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=370355097&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=370355097&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=370355097&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=370355097&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1397424590
> RE 9.1.1 eh... I wouldn't bother, personally. Let's see if a more serious concern is brought up.
This makes sense to me. I suspect this has been a (mostly) latent issue for a while now. SOLR-15337 may have made it more apparent in some ways, but I think this type of issue existed, even in this part of the codebase, even before that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081395936
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
} catch (IOException e) {
throw new SolrException(ErrorCode.SERVER_ERROR, e);
+ } finally {
+ // according to spec, XML parser should close InputStream when parsing is complete.
+ // But in the event that this doesn't happen (either because an exception is
+ // thrown or because of an error in parser implementation, here we ensure that it
+ // is closed.
Review Comment:
addressed in 9507e1ae3be06daaf7400fa41e77a75d3fb55981
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081644200
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
To be clear, I'm really okay with that change. You've added it I see; it's fine.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081919641
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -48,6 +50,7 @@
import org.xml.sax.InputSource;
/** Factory for ManagedIndexSchema */
+@NotThreadSafe
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081618038
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
Thanks for the reassurance and extra explanation!
3a047d404b4e994865711229329a7461a751897b should fix this. Whether or not this class needs to be thread-safe, it seems weird to me that `loadedResource` is an instance variable -- seems mostly to exist in order to pass extra info from the `readSchemaLocally()` method back to the `create()` method, which could be straightforwardly done another way. I'm happy to include 3a047d404b4e994865711229329a7461a751897b in this PR or revert it from the PR branch, depending on what you prefer.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081903901
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -48,6 +50,7 @@
import org.xml.sax.InputSource;
/** Factory for ManagedIndexSchema */
+@NotThreadSafe
Review Comment:
`noggit.JSONParser.devNull.buf`? Not seeing this anywhere ... even indirectly, I only see XML parsing here. Not sure where JSON anything is coming from.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081694315
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +196,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ String[] loadedResourceRef = new String[1];
+ schemaInputStream = readSchemaLocally(loadedResourceRef);
Review Comment:
Lets add NotThreadSafe annotation to the class; ehh?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1088145382
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -290,8 +301,9 @@ public ManagedIndexSchema create(
return schema;
}
- private InputStream readSchemaLocally() {
+ private InputStream readSchemaLocally(String[] loadedResourceRef) {
Review Comment:
Couldn't we return a class or record or something else that you could pull the `InputStream` and `loadedResource` out of? This is a private method so we can modify however we want. no need to have the whole array stuff?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081919327
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -48,6 +50,7 @@
import org.xml.sax.InputSource;
/** Factory for ManagedIndexSchema */
+@NotThreadSafe
Review Comment:
In any event based on the entirely no-op impl of `noggit.CharArr.NullCharArr` (of which `JSONParser.devNull` is an instance -- `devNull` again lol), I'm pretty sure that this is perfectly fine. The signatures of the methods to which `JSONParser.devNull` is passed accept CharArr, and I guess lift-bot doesn't recognize that in the case of `devNull` there's actually no chance of modifying the backing `buf` (b/c write methods are all no-ops).
So I think this is safe to ignore.
@sonatype-lift ignore
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081515055
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
This is real. No idea if the way this is called in practice would ever make this problematic, but in any event `loadedResource` can be trivially converted to a local variable; there's no reason for it to be an instance variable. Now let's see if lift-bot complains about the latent NPE risk that the fix makes more apparent đ
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081543949
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
Review Comment:
This is an optional feedback mechanism for them to improve Lift. I clicked "Not relevant".
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081572121
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
I don't think a SchemaFactory needs to be thread-safe. I see that they are always created in order to call create() and then are thrown away, not saved for potential re-use. Ideally if we annotate this class with `net.jcip.annotations.NotThreadSafe` one would hope tooling could be smarter to understand that THREAD_SAFETY_VIOLATION is impossible here. Lift doesn't do its own analysis, it delegates to tools like "Infer" which has a module "RacerD" that implements this particular THREAD_SAFETY_VIOLATION analysis. I was looking into this last week -- it's kind of fascinating. I want to help RacerD help Lift help us. When I look at RacerD docs, it sadly seems a bit dated; doesn't recognized popular annotations like NotThreadSafe; it has its own. In this case, an annotation [ThreadConfined](https://fbinfer.com/docs/1.0.0/checker-racerd/#threadconfined) should be equivalent. I'll report an issue there and to Lift as well.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1397415044
RE 9.1.1 eh... I wouldn't bother, personally. Let's see if a more serious concern is brought up.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081700035
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +196,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ String[] loadedResourceRef = new String[1];
+ schemaInputStream = readSchemaLocally(loadedResourceRef);
Review Comment:
Yeah, already doing so đ ... given that I'm doing so, would you prefer that I revert 3a047d404b4e994865711229329a7461a751897b ? My inclination is to leave it, since I don't like the way the instance variable was being used to basically pass a return value from a method (a different case from the most recent complaint about `managedSchemaResourceName`, which is accessed externally).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081362734
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
Review Comment:
ah, thanks for looking into it. Then "not relevant", or "won't fix", would you say?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081568819
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -393,14 +393,22 @@ private SolrConfig(
}
private IndexSchemaFactory.VersionedConfig readXml(SolrResourceLoader loader, String name) {
+ InputStream in = null;
try {
- ResourceProvider rp = new ResourceProvider(loader, name);
+ in = loader.openResource(name);
+ ResourceProvider rp = new ResourceProvider(in);
XmlConfigFile xml = new XmlConfigFile(loader, rp, name, null, "/config/", null);
return new IndexSchemaFactory.VersionedConfig(
rp.zkVersion,
new DataConfigNode(new DOMConfigNode(xml.getDocument().getDocumentElement())));
Review Comment:
@sonatype-lift ignore
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081655354
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -198,88 +198,94 @@ public ManagedIndexSchema create(
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ schemaInputStream = readSchemaLocally();
+ } else { // ZooKeeper
+ final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
+ final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
+ final String managedSchemaPath = lookupZKManagedSchemaPath();
+ managedSchemaResourceName =
+ managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
+ Stat stat = new Stat();
try {
- schemaInputStream = loader.openResource(resourceName);
- loadedResource = resourceName;
- shouldUpgrade = true;
- } catch (IOException e) {
+ // Attempt to load the managed schema
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream =
+ new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
+ loadedResource = managedSchemaResourceName;
+ warnIfNonManagedSchemaExists();
+ } catch (InterruptedException e) {
+ // Restore the interrupted status
+ Thread.currentThread().interrupt();
+ log.warn("", e);
+ } catch (KeeperException.NoNodeException e) {
+ log.info(
+ "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
+ managedSchemaResourceName,
+ resourceName);
+ } catch (KeeperException e) {
+ String msg = "Error attempting to access " + managedSchemaPath;
+ log.error(msg, e);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ }
+ if (null == schemaInputStream) {
+ // The managed schema file could not be found - load the non-managed schema
try {
- // Retry to load the managed schema, in case it was created since the first attempt
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream = new ByteArrayInputStream(data);
- loadedResource = managedSchemaPath;
- warnIfNonManagedSchemaExists();
- } catch (Exception e1) {
- if (e1 instanceof InterruptedException) {
- Thread.currentThread().interrupt(); // Restore the interrupted status
+ schemaInputStream = loader.openResource(resourceName);
+ loadedResource = resourceName;
+ shouldUpgrade = true;
+ } catch (IOException e) {
+ try {
+ // Retry to load the managed schema, in case it was created since the first attempt
+ byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
+ schemaZkVersion = stat.getVersion();
+ schemaInputStream = new ByteArrayInputStream(data);
+ loadedResource = managedSchemaPath;
+ warnIfNonManagedSchemaExists();
+ } catch (Exception e1) {
+ if (e1 instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore the interrupted status
+ }
+ final String msg =
+ "Error loading both non-managed schema '"
+ + resourceName
+ + "' and managed schema '"
+ + managedSchemaResourceName
+ + "'";
+ log.error(msg, e);
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
- final String msg =
- "Error loading both non-managed schema '"
- + resourceName
- + "' and managed schema '"
- + managedSchemaResourceName
- + "'";
- log.error(msg, e);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg, e);
}
}
}
+ InputSource inputSource = new InputSource(schemaInputStream);
+ inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(loadedResource));
Review Comment:
Yeah it's fine.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1081712491
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +196,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ String[] loadedResourceRef = new String[1];
+ schemaInputStream = readSchemaLocally(loadedResourceRef);
Review Comment:
Um; it's really fine the way it is. Balancing two concerns. I like fewer fields!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1302:
URL: https://github.com/apache/solr/pull/1302#issuecomment-1397459619
CHANGES.txt -- I recommend adding an entry. It is a bug, albeit minor.
I recommend waiting until after 9.1.1 vote passes before merging or adding CHANGES.txt. There is a chance we'll pull it into 9.1.1 even though at this moment we think not.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1100587425
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +200,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ Entry<String, InputStream> localSchemaInput = readSchemaLocally();
Review Comment:
It sucks to still see this because we have a NotThreadSafe annotation, thus I wasn't anticipating the checks. From reading the [Infer/RacerD docs in more detail](https://fbinfer.com/docs/1.0.0/all-issue-types/#thread-safety-conditions-checked-before-reporting), it appears this is because this class, despite **not** being ThreadSafe, and advertising itself as such nonetheless contains the "synchronized" keyword in one spot at the end of create(). The presence of the keyword in the code takes precedence over NotThreadSafe annotation. It seems to me we don't need that synchronized there because the schema is not published yet. Any way, such changes / explorations don't belong here. I'll create a PR to toy with this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1100712374
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -197,89 +200,99 @@ public ManagedIndexSchema create(
this.config = config;
this.loader = config.getResourceLoader();
InputStream schemaInputStream = null;
+ String loadedResource = null;
- if (null == resourceName) {
- resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
- }
-
- int schemaZkVersion = -1;
- if (!(loader instanceof ZkSolrResourceLoader)) {
- schemaInputStream = readSchemaLocally();
- } else { // ZooKeeper
- final ZkSolrResourceLoader zkLoader = (ZkSolrResourceLoader) loader;
- final SolrZkClient zkClient = zkLoader.getZkController().getZkClient();
- final String managedSchemaPath = lookupZKManagedSchemaPath();
- managedSchemaResourceName =
- managedSchemaPath.substring(managedSchemaPath.lastIndexOf("/") + 1); // not loving this
- Stat stat = new Stat();
- try {
- // Attempt to load the managed schema
- byte[] data = zkClient.getData(managedSchemaPath, null, stat, true);
- schemaZkVersion = stat.getVersion();
- schemaInputStream =
- new ZkSolrResourceLoader.ZkByteArrayInputStream(data, managedSchemaPath, stat);
- loadedResource = managedSchemaResourceName;
- warnIfNonManagedSchemaExists();
- } catch (InterruptedException e) {
- // Restore the interrupted status
- Thread.currentThread().interrupt();
- log.warn("", e);
- } catch (KeeperException.NoNodeException e) {
- log.info(
- "The schema is configured as managed, but managed schema resource {} not found - loading non-managed schema {} instead",
- managedSchemaResourceName,
- resourceName);
- } catch (KeeperException e) {
- String msg = "Error attempting to access " + managedSchemaPath;
- log.error(msg, e);
- throw new SolrException(ErrorCode.SERVER_ERROR, msg, e);
+ try {
+ if (null == resourceName) {
+ resourceName = IndexSchema.DEFAULT_SCHEMA_FILE;
}
- if (null == schemaInputStream) {
- // The managed schema file could not be found - load the non-managed schema
+
+ int schemaZkVersion = -1;
+ if (!(loader instanceof ZkSolrResourceLoader)) {
+ Entry<String, InputStream> localSchemaInput = readSchemaLocally();
Review Comment:
Ah, ok, that makes sense! Thanks for following up on this. I expect to merge/backport tomorrow.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1090073943
##########
solr/core/build.gradle:
##########
@@ -115,6 +115,8 @@ dependencies {
implementation 'commons-cli:commons-cli'
+ implementation 'net.jcip:jcip-annotations'
Review Comment:
@magibney lets remove the use of such annotations in this PR. In #1313 I added it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1088145382
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -290,8 +301,9 @@ public ManagedIndexSchema create(
return schema;
}
- private InputStream readSchemaLocally() {
+ private InputStream readSchemaLocally(String[] loadedResourceRef) {
Review Comment:
Couldn't we return a class or record or something else that you could pull the `InputStream` and `loadedResource` out of? This is a private method so no need to have the whole array stuff?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1302: SOLR-16628: Ensure that InputStreams are closed after Xml parsing
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1302:
URL: https://github.com/apache/solr/pull/1302#discussion_r1089762660
##########
solr/core/build.gradle:
##########
@@ -115,6 +115,8 @@ dependencies {
implementation 'commons-cli:commons-cli'
+ implementation 'net.jcip:jcip-annotations'
Review Comment:
So maybe @dsmiley knows which `jcip-annotations` is correct.
At least based on https://github.com/stephenc/jcip-annotations - it should be https://mvnrepository.com/artifact/com.github.stephenc.jcip/jcip-annotations/1.0-1
however it looks like we include `net.jcip:jcip-annotations` already somehow?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org