Re: [apache/struts] WW-4920 fix java.net.JarURLConnection#parseSpecs (#209)

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

Re: [apache/struts] WW-4920 fix java.net.JarURLConnection#parseSpecs (#209)

Lukasz Lenart
I moved discussion here to avoid notify other Apache committers. I think
you should close the PR and open a new one :)

2018-02-19 21:58 GMT+01:00 Yasser Zamani <[hidden email]>:

> 4b89034
> <https://github.com/apache/struts/commit/4b890345e8b7a7f760b170672782296e9113d445>
> has deleted if (isReloadingConfigs()) { from all implemented
> FileManager#monitorFile methods. This causes Struts to monitor all loaded
> files always, regardless of if the value of STRUTS_CONFIGURATION_XML_RELOAD
> = "struts.configuration.xml.reload" is true or false. i.e. Struts may
> reload a file even when that constant is set to false because if
> (revision == null) { is false and Struts will use monitored revision in
> FileManager#fileNeedsReloading. As FileManager interface documents:
>
>     /**     * Adds file to list of monitored files if {@link #setReloadingConfigs(boolean)} is true     *     * @param fileUrl {@link URL} to file to be monitored     */
>     void monitorFile(URL fileUrl);
>
> I think we must revert this change (other changes of this commit looks
> good). I'm just worry about:
>
>         //watch class file
>         if (isReloadEnabled()) {
>             URL classFile = actionClass.getResource(actionClass.getSimpleName() + ".class");
>             fileManager.monitorFile(classFile);
>             loadedFileUrls.add(classFile.toString());
>         }
>
> in \convention\PackageBasedActionConfigBuilder.java. This piece of code
> seems expects monitorFile method always monitor but I think if it wants to
> monitor .class files, then it must use it's own internal mechanism
> because as documents say, the monitorFile method will monitor if
> STRUTS_CONFIGURATION_XML_RELOAD = "struts.configuration.xml.reload" is
> set to true by user.
>
> Any way I feel OK to revert back that if statement and adding more heavy
> test covering all possible situations i.e. when that constant is set to
> true or false (two states) and when file is modified or not (two states) so
> total four states. @lukaszlenart <https://github.com/lukaszlenart> ,
> @apache/apache-committers
> <https://github.com/orgs/apache/teams/apache-committers> ❓
>
>
This is not needed as the reload check was moved into
FileManager#fileNeedsReloading methods - see usage of those methods.


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
Reply | Threaded
Open this post in threaded view
|

Re: [apache/struts] WW-4920 fix java.net.JarURLConnection#parseSpecs (#209)

Yasser Zamani-2


On 2/20/2018 10:13 AM, Lukasz Lenart wrote:
> I moved discussion here to avoid notify other Apache committers. I think
> you should close the PR and open a new one :)

Yes I closed and also locked that :) I'm sorry and embarrassed if I
harmed Struts :( As a user, I wished GitHub at least show a warn message
when at-sign has such a lot of recipients.

> This is not needed as the reload check was moved into
> FileManager#fileNeedsReloading methods - see usage of those methods.

Yes I saw. However, FileManager#fileNeedsReloading method depends on if
`revision = files.get` is null or not i.e. depends on if the file is
under monitor or not, and as loadFile always calls monitorFile then
currently, every loaded file is under monitor regardless of if user has
set `struts.configuration.xml.reload` constant to true or not.

Regards.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [apache/struts] WW-4920 fix java.net.JarURLConnection#parseSpecs (#209)

Lukasz Lenart
2018-02-20 10:21 GMT+01:00 Yasser Zamani <[hidden email]>:
>> This is not needed as the reload check was moved into
>> FileManager#fileNeedsReloading methods - see usage of those methods.
>
> Yes I saw. However, FileManager#fileNeedsReloading method depends on if
> `revision = files.get` is null or not i.e. depends on if the file is
> under monitor or not, and as loadFile always calls monitorFile then
> currently, every loaded file is under monitor regardless of if user has
> set `struts.configuration.xml.reload` constant to true or not.

but FileManager#fileNeedsReloading won't be called if
"struts.configuration.xml.reload" is set to false, see
ConfigurationManager#conditionalReload


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [apache/struts] WW-4920 fix java.net.JarURLConnection#parseSpecs (#209)

Yasser Zamani


On 2/20/2018 1:37 PM, Lukasz Lenart wrote:

> 2018-02-20 10:21 GMT+01:00 Yasser Zamani <[hidden email]>:
>>> This is not needed as the reload check was moved into
>>> FileManager#fileNeedsReloading methods - see usage of those methods.
>>
>> Yes I saw. However, FileManager#fileNeedsReloading method depends on if
>> `revision = files.get` is null or not i.e. depends on if the file is
>> under monitor or not, and as loadFile always calls monitorFile then
>> currently, every loaded file is under monitor regardless of if user has
>> set `struts.configuration.xml.reload` constant to true or not.
>
> but FileManager#fileNeedsReloading won't be called if
> "struts.configuration.xml.reload" is set to false, see
> ConfigurationManager#conditionalReload

Thanks Łukasz! you were right. Then to sum up, Struts monitors all
loaded files regardless of devMode or "struts.configuration.xml.reload"
values (i.e. user may get a warning exception if framework was not able
to create a revision for loaded file). But Struts checks if reload is
needed only when "struts.configuration.xml.reload" constant is set to
true (devMode still doesn't make sense). Looks good to me however it
seems javadocs of FileManager interface should being fixed to satisfy these.

Regards.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [apache/struts] WW-4920 fix java.net.JarURLConnection#parseSpecs (#209)

Lukasz Lenart
2018-02-20 12:32 GMT+01:00 Yasser Zamani <[hidden email]>:
> Thanks Łukasz! you were right. Then to sum up, Struts monitors all
> loaded files regardless of devMode or "struts.configuration.xml.reload"
> values (i.e. user may get a warning exception if framework was not able
> to create a revision for loaded file). But Struts checks if reload is
> needed only when "struts.configuration.xml.reload" constant is set to
> true (devMode still doesn't make sense). Looks good to me however it
> seems javadocs of FileManager interface should being fixed to satisfy these.

"monitor" is a wrong word here, it keeps reference to the file ;-) I
would fix it as simple as possible and consider large refactor in 2.6.


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]