WW-4948 delete temp files on close instead of on JVM exists

[yasserzamani]

No description provided.

[coveralls]

Coverage increased (+0.02%) to 46.835% when pulling b816cc1 on yasserzamani:WW-4948 into daac47e on apache:struts-2-5-x.

[lukaszlenart]

Looks good to me 👍 if no objections it can be merged.

[yasserzamani]

@apache/struts-committers , I and also the issue reporter are agree.

[lukaszlenart]

No objections, LGTM 👍

[lukaszlenart]

Can you port this into master branch?

[lukaszlenart]

This fix introduced a small problem struts-community-plugins/struts2-jquery#165

[sdutry]

minimal project for this new issue:
https://github.com/sdutry/struts2-staticFiles-devMode

Starting up the jetty server ( mvn jetty:run ) and going to http://localhost:8080/index.action shows you it fails to load the styles.css and utils.js resources (originating from the <s:head /> tag).

problem:
loading a static resource with struts.devMode set to true

It throws a NullPointerException in DefaultStaticContentLoader because it creates a new instance on each request, but doesn't call setHostConfig other than on the initialy created object => leaving pathPrefixes null .

in version 2.5.17 it was the same object and not a new instance

[lukaszlenart]

It's because we properly reloads JAR now, but I think this can be improved by initiating FileManager in a proper way, right now it will reload everything at least once even if not needed.

[lukaszlenart]

Hmm... Yasser's change looks good, I have no idea why this could break anything.

• revisions are created only if reloadConfigs is set to true (in devMode)
• then FileManager checks if given file need to be reloaded (and this works ok)

I cannot reproduce the issue with my local AppEngine setup.

[yasserzamani]

Thanks a lot for investigations and samples! I continued with them and finally found the bug which to resolve, we should think about the question "Who came first egg or hen?":

1. Firstly Struts itself loads it's internal configuration with DefaultFileManager#28 reloadingConfigs = false so with 2.5.18, it correctly doesn't (and shouldn't) create revision for struts-default.xml and struts.xml (See DefaultFileManager#51).
2. struts.xml processing then sets reloadingConfigs = true according to user demand!
3. In result, ConfigurationManager#128 always check if reload which reaches if struts-default.xml needs reload which reaches DefaultFileManager#43 return revision == null ? this.reloadingConfigs : revision.needsReloading(); which will be evaluated to reloadingConfigs i.e. true because there are no revision.
4. Then Struts so reloads struts-default.xml and creates a new incomplete instance of DefaultStaticContentLoader.

Actually I think DefaultFileManager#43 return revision == null ? this.reloadingConfigs : revision.needsReloading(); which is from past is somehow wrong or I cannot find what the writer tried to achieve by it. Maybe by reloadingConfigs = true meant always reloading config. I afraid!! but If so, then it's the DefaultStaticContentLoader which cannot being reloaded and then same issue may occur similarly for other Struts internal singleton beans which their state has been changed.

Kind Regards :)

[sdutry]

@yasserzamani
Does this mean that certain classes should not be reloaded even if reloadingConfigs is true or does this mean that they should be able to and therefore should be fully re-initialised when it happens?

(sorry if it's a dumb question, i've not been active on the project for quite some time, trying to get back into it)

[yasserzamani]

@sdutry , you're welcome back thank you very much :) No it's a good question.

I rethought and am thinking about these. According to last thoughts, It seems as currently already behavior, it's nice and OK to load Struts internal configuration i.e. struts-default.xml with reloadingConfigs=false rather if user has set it or devMode to true in his/her app config or not. Until this, Struts is correct and doesn't create a revision for struts-default.xml, which I like, because it's not a good practice to monitor it for changes and reload!; It always is not modified even between some Struts versions!

So I think the problem is with DefaultFileManager.java#L63. When user sets reloadingConfigs=true then that line always return true then Struts reloads it's internal struts-default.xml which not only is a bad practice, but also even isn't needed as it's not modified.

Kind regards.

[lukaszlenart]

but when reloadConfigs is true, the revision is there DefaultFileManager.java#L73-L75, so this shouldn't be the case

[yasserzamani]

No. If a break point putted there, it's false firstly then after processing user xml, it will be set to true. For struts-default and user struts xml URLs it's false even if user has set it true. So a revision will not created for them. For struts-default xml this is a good behaviour but isn't for user xml and etc.