[GitHub] tomee pull request #176: TOMEE-2253 - tomee.sh -version not working properly...

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #176: TOMEE-2253 - tomee.sh -version not working properly...

deki
GitHub user danielsoro opened a pull request:

    https://github.com/apache/tomee/pull/176

    TOMEE-2253 - tomee.sh -version not working properly with Java 11

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/danielsoro/tomee tomee-2253

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/tomee/pull/176.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #176
   
----
commit bafe834710301e44b8d9d56f0c3a9340cb319314
Author: Daniel Cunha (soro) <danielsoro@...>
Date:   2018-10-09T14:56:45Z

    TOMEE-2253 - tomee.sh -version not working properly with Java 11

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @rmannibucau  What do you want mean: should likely be closed?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @danielsoro classloader.close() must be called to not leak and enable to read files after the end of the container.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @rmannibucau hmm.. my question is: If we need to call close, why are not we doing it when using CustomizableURLClassLoader? is it means which we need to do the same for CustomizableURLClassLoader?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Java 6 didnt have that method but we should close all our classloaders once yes.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Hey @rmannibucau,
   
    I updated it to call `close` and also now we call the ClassLoader.registerAsParallelCapable(); in a static block.
   
    hashcode and equals still using the super, the new class doesn't have any property which we can use to create a custom equals ou hashcode. If you have some idea how can I create ones, please show off.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    well some thoughts:
   
    1. most of the time we dont need this dynamism so maybe we can just rework the distro and delete it
    2. we already hack equals/hashcode in cxf container loader but it is not super robust (this is why we have a flag to disable it) so maybe not the best solution but current impl clearly breaks some apps
    3. you can open java modules to get the URLClassPath, even of the system classloader in j11 so maybe we just document how to open it and replace the cast by some reflection?
   



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @rmannibucau,
   
    TomEE is not showing any exception when running with j11 (I was able to start and deploy a simple application on it really fine), that is not the issue.
   
    It just appears with tomee.sh when I'm using the CLI commands.
    Maybe applying that change just to cover the org.apache.openejb.cli.Bootstrap should works.
   
    WDYT?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Yes, this is what i had in mind as well. That said if it is just for our CLI your hack is fine, we should just lock it for other cases like openejb-standalone


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @rmannibucau,
   
    Nice.. working on it.
    Thank you! :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Hi @rmannibucau,
   
    patch updated. It seems to look better now.
    Please, review it and thank you for all the help, I really appreciate it.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Hope i didnt misread the diff (starts to be late ;)) but it misses a close and the urlclassloader cast will not work on all java versions right?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Ohh.. sorry I sent the wrong patch.
    Pushing the correct one.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Ok @rmannibucau,
   
    sent the correct patch, let me know what do you think.
    Thank you!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Hmm, you close the classloader before it is used so it is no more usable normally - or it will leak.
   
    Side question: any issue having an impl os ClassPath using the classloader to keep existing one for java 8 users?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @rmannibucau,
   
    Yeah, I'm closing it before, going to fix it.
    I didn't get your question about j8's user, but it should not have an impact on j8's user.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @danielsoro if you change the classloader it can impact the users relying on that (it can be the case if they tune java or use system classloader only libs. This is rare but already saw it. This is why i think it can be worth not changing the default behavior.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    @rmannibucau I get it, but we isolate it just for the CLI.
   
    What is the scenario which you are seeing which can break something with that change? It should not have any impact on the application deployed in TomEE.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Thnking to users having a java -cp command launching Cli (we have a few).
   
    Side note: i think you still close the classloader before it is finished to be used - after cli execution pby


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #176: TOMEE-2253 - tomee.sh -version not working properly with J...

deki
In reply to this post by deki
Github user danielsoro commented on the issue:

    https://github.com/apache/tomee/pull/176
 
    Well.. it is calling inside a try-statement, I believe which the close will be called in the end of the program or when some exception occur: https://github.com/apache/tomee/pull/176/files#diff-78e3cde3f4b438e49a55568e9cee40d9R162
   
    About java -cp.. I'm going to try provide a test for it.


---
12