You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/12/09 01:28:49 UTC
[GitHub] [logging-log4j2] raipc opened a new pull request, #1164: Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
raipc opened a new pull request, #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164
This PR resolves Log4j initialization problem in Intellij IDEA plugins which use Log4j2 for logging
```
Caused by: java.lang.ClassCastException: class com.intellij.util.lang.ZipResourceFile$MyUrlConnection cannot be cast to class java.net.JarURLConnection (com.intellij.util.lang.ZipResourceFile$MyUrlConnection is in unnamed module of loader 'app'; java.net.JarURLConnection is in module java.base of loader 'bootstrap')
at org.apache.logging.log4j.core.config.ConfigurationSource.getConfigurationSource(ConfigurationSource.java:372)
```
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a diff in pull request #1164: [LOG4J2-3047] Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
rgoers commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1047167520
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
@ppkarwasz Yes, that sounds like a better solution. This merge should be reverted.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1164: [LOG4J2-3047] Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1047194010
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
@rgoers: my point is that `urlConnection instanceof JarURLConnection` is probably the right thing to do here.
As for the JDK bug: [JDK-6956385](https://bugs.openjdk.java.net/browse/JDK-6956385) has been reported so many years ago, that it should probably be considered a feature by now.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1164: Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#issuecomment-1345391062
Nice catch! Thanks for the PR.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] raipc commented on a diff in pull request #1164: Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
raipc commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1046566581
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
1. The connection implementation is chosen by ClassLoader. It may be different from java.net.JarURLConnection. See the exception provided in the PR description.
2. I see no reason to duplicate this check. Even if JarURLConnection will handle other protocol, no harm will be made by the workaround
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LOG4J2-3047] Check urlConnection for being instance of JarURLConnection instead of relying on protocol name (logging-log4j2)
Posted by "jglick (via GitHub)" <gi...@apache.org>.
jglick commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1171847447
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
https://github.com/openjdk/jdk/pull/12871 should fix 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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1164: [LOG4J2-3047] Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1046674709
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
The problem with `sun.net.www.protocol.jar.JarURLConnection` was that it leaked file descriptors upon each call to `getLastModified()` (cf. [LOG4J2-3423](https://issues.apache.org/jira/browse/LOG4J2-3423)).
I am not sure why IntelliJ decided not to extend the abstract class `java.net.JarURLConnection`, but I guess we don't need any workaround with classes that do not extend `java.net.JarURLConnection`.
BTW: this problem was already reported as [LOG4J2-3047](https://issues.apache.org/jira/browse/LOG4J2-3047).
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a diff in pull request #1164: Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
rgoers commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1046648471
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
The problem I have is that this change means only JarURLConnection is supported even if something else is implementing the JAR protocol. Instead, we should be investigating why the ClassCastException is occurring and address 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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a diff in pull request #1164: [LOG4J2-3047] Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
rgoers commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1047167520
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
@ppkarwasz Yes, that sounds like a better solution. This merge should be reverted. Also, if the call to getLastModified is leaking file descriptors shouldn't an issue be raised with the JDK 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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz merged pull request #1164: Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
jvz merged PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a diff in pull request #1164: Check urlConnection for being instance of JarURLConnection instead of relying on protocol name
Posted by GitBox <gi...@apache.org>.
rgoers commented on code in PR #1164:
URL: https://github.com/apache/logging-log4j2/pull/1164#discussion_r1044551781
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -367,7 +357,7 @@ private static ConfigurationSource getConfigurationSource(URL url) {
try {
if (file != null) {
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
- } else if (JAR.equals(url.getProtocol())) {
+ } else if (urlConnection instanceof JarURLConnection) {
Review Comment:
1. Why is it necessary to check the class instead of the protocol?
2. If we add this check why wouldn't we still check the protocol also?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org