Closed javaserverfaces closed 8 years ago
Reported by lprimak
lprimak said: Just tested a very simple fix:
Pull request (updated) https://github.com/javaserverfaces/mojarra/pull/1
Changes: https://github.com/javaserverfaces/mojarra/pull/1/files?diff=split
lprimak said: Diffs:
diff --git a/jsf-ri/src/main/java/com/sun/faces/facelets/impl/DefaultFaceletCache.java b/jsf-ri/src/main/java/com/sun/faces/facelets/impl/DefaultFaceletCache.java
index 935489f..ad1cbe8 100644
--- a/jsf-ri/src/main/java/com/sun/faces/facelets/impl/DefaultFaceletCache.java
+++ b/jsf-ri/src/main/java/com/sun/faces/facelets/impl/DefaultFaceletCache.java
@@ -201,6 +201,11 @@ final class DefaultFaceletCache extends FaceletCache<DefaultFacelet> {
long getNextRefreshTime() {
// There is no point in calculaing the next refresh time if we are refreshing always/never + return (_refreshInterval > 0) ? _nextRefreshTime.get() : 0;
+ }
+
+ long getAndUpdateNextRefreshTime() {
+ // There is no point in calculaing the next refresh time if we are refreshing always/never
return (_refreshInterval > 0) ? _nextRefreshTime.getAndAdd(_refreshInterval) : 0;
}
@@ -215,9 +220,8 @@ final class DefaultFaceletCache extends FaceletCache<DefaultFacelet> {
@Override
public boolean isExpired(URL url, Record record) {
- // getNextRefreshTime() incremenets the next refresh time atomically - long ttl = record.getNextRefreshTime();
- if (System.currentTimeMillis() > ttl) {
+ if (System.currentTimeMillis() > record.getNextRefreshTime()) {
+record.getAndUpdateNextRefreshTime();
long lastModified = Util.getLastModified(url);
// The record is considered expired if its original last modified time
// is older than the URL's current last modified time
@arjantijms said: Applied patch to 2.3 master
git push Counting objects: 12, done. Delta compression using up to 8 threads. Compressing objects: 100% (8/8), done. Writing objects: 100% (12/12), 924 bytes | 0 bytes/s, done. Total 12 (delta 6), reused 0 (delta 0) To ssh://arjan_t@git.java.net/mojarra~git 4f075ce..aefec8a master -> master
lprimak said: Thanks Arjan, should I close the pull request? Has this been incorporated into github? Thanks
@arjantijms said: Hi Lenny, yeah, the pull request can be closed. https://github.com/javaserverfaces is a read-only "end" mirror. I explained the organisation a little bit over here: https://github.com/jsf-spec/mojarra-readme
The patch has been incorporated into Github indeed, see here: https://github.com/javaserverfaces/mojarra/blob/master/jsf-ri/src/main/java/com/sun/faces/facelets/impl/DefaultFaceletCache.java#L195
Thanks for the contribution
mmariotti said: Hi guys, What's the conceptual meaning of incrementing the threshold?
Shouldn't it be:
long getAndUpdateNextRefreshTime() {
// There is no point in calculating the next refresh time if we are refreshing always/never // There is no point in incrementing either, just SET it to expire from NOW! return (_refreshInterval > 0) ? _nextRefreshTime.getAndSet(System.currentTimeMillis() + _refreshInterval) : 0;
}
lprimak said: I think your way would work as well, although untested. When I made my changes I wanted make least impact and introduce least amount of risk. Perhaps there is something to that and I didn't realize it. Bottom line that it works correctly this way, and there is no reason to change it unless you want to re-test a whole bunch of stuff. Your way is no more efficient or correct than mine, IMHO.
mmariotti said: I'll understand your reasons, but there is a corner (although very common) case where your way is not efficient. I'll try to show why:
suppose 't0' is any instant on timeline:
**now: t0**create facelet record and put in cache
record._creationTime := t0
record._lastModified := t0 - 10,000ms (arbitrarily chosen in the past)
record._refreshInterval := 2,000ms (arbitrarily chosen)
record._nextRefreshTime := t0 + 2,000ms
then, suppose that this facelet is requested again after 1,000,000ms and has not been modified on filesystem:
**now: t0 + 1,000,000ms**get/read record from cache
record._creationTime = t0
record._lastModified = t0 - 10,000ms
record._refreshInterval = 2,000ms
record._nextRefreshTime = t0 + 2,000ms
check for expiration:
record._nextRefreshTime' < 'now' ==>
t0 + 2,000ms < t0 + 1,000,000ms ==>
expired!
update: record._nextRefreshTime := t0 + 2,000ms + 2,000ms = t0 + 4,000ms
check file modification: Util.getLastModified(url) (heavy operation!) ==> result: not modified
now suppose that record is accessed again and again, with 1ms interval between accesses (just point the first):
**now: t0 + 1,000,001ms**get/read record from cache
record._creationTime = t0
record._lastModified = t0 - 10,000ms
record._refreshInterval = 2,000ms
record._nextRefreshTime = t0 + 4,000ms
check for expiration:
record._nextRefreshTime < 'now' ==>
t0 + 4,000ms < t0 + 1,000,001ms ==>
expired! ==>
WRONG!
The record is seen as expired even if it has been accessed (and found expired - then 'refreshed') 1ms ago.
This cause Util.getLastModified(url) (heavy op) to be called on each subsequent access (including this access).
And this will continue on, until the access threshold is included within ('now'; 'now' + '_refreshInterval'] (depending on '_refreshInterval', this may also be impossible?).
So, this algo is not efficient. And it's because there's no conceptual reason to 'increment' the threshold. Instead, just resetting the threshold on expire (and expunging the record if modified) is sufficient to maintain the algo safe and make it more efficient.
The concepual constraint, in this case, is: 'let x be a record: there is at most one expiration in any possible refresh interval, for record x'. In other words: if record x has expired less than '_refreshInterval' ms ago, it's useless to check it again.
Although there's no formal proof of correctness, I don't see any side effect in resetting the threshold - no invalid case comes in my mind - maybe your eyes can spot something wrong yet.
If you are concerned about testing, I'll be glad to help you as much as I can.
Thank you, and keep up with the good work
lprimak said: Is this a theoretical issue you have discovered or is there an actual problem you are trying to solve in production software? There are many, many theoretical efficiency issues throughout world's codebases that aren't being fixed either. If this is an actual issue, please submit a separate enhancement request.
This particular issue was brought up because an actual bug encountered in production software. It was fixed with minimal changes in the code or algorithms to lower the risk of regression.
Saying all that, you are probably right and your explanation is great, but I have no time currently to do a bunch of regression testing on this issue that is now working for months and months. (and I am no longer on that project)
lprimak said: I also not sure what the original author's code intent was. There, perhaps was a direction that you are not seeing, or perhaps not, since I am not the original author, I simply do not know.
lprimak said: Bottom line, if you want to improve the code, please open up another issue, and get some regression tests going.
mmariotti said: It's both theoretical and practical: if you declare a FACELET_REFRESH_PERIOD > 0 (or not declaring it at all, defaulting to 2s) the case I shown you before is easily encountered, leading to production-deployed apps performance issues. Conversely to the issue you reported (facelets do not reload), applying the patch provided generates the side-effect that reload-check happens too often. Generally speaking this is a major issue only if your app makes extensive use of composite components, templates and/or fragment includes. In all other cases the performance impact is negligible.
Anyway, I cloned this issue to: https://java.net/jira/browse/JAVASERVERFACES-4176 for the side-effect, I'll update it with the results we found.
lprimak said: Instead of cloning an existing issue, which it clearly isn't, you should close the clone and create a brand new issue. Your issue has nothing to do with cache not expiring. Your issue deals with it expiring too often.
I would really suggest writing an actual test that demonstrates the issue, otherwise it's doubtful it will be looked at. I have tested the running code manually with 5 min refresh interval and it worked as expected. Your reasoning is sound by looking at the code, but I think there is something both you and I don't understand in the code's behavior because it works correctly in my (albeit not truly automatic, i.e. manual) tests.
mmariotti said: Sincerly, I'm not really interested in the resolution/optimization of this issue anymore, I just wanted to point it out for other people that could hit this. Meanwhile I re-implemented all the facelet cache stack from scratch for both development and production, and I'll stick to it in the future.
webelcomau said: From (otherwise) a big fan of JSF.
This facelet caching problem has been an enormously inconvenient and expensive problem for me (as described here). The symptoms as they expressed themselves in a large web app for me with 2.2.8-12 and then 2.2.8-17 (and now it seems 2.2.8-18 from 18 Aug 2016) are:
On page reload in large JSF2 web apps that use lots of composite components, facelet reloading of composite components is getting "stuck" (in some even cases for minutes)
Yes, that was minutes. Sometimes for 10 minutes or longer. It completely prevents sensible JSF development. Redeploy, wait. Redeploy, wait. Redeploy, wait. (It's the kind of situation some PHP fans use to poke fun at Java.)
As far as I can tell, it is NOT fixed in 2.2.8-17, 2.2.8-18.
The FIX version is indicated as 2.3.0-m06. Unfortunately, I can't run the web app I am developing against 2.3.0-m06, it simply crashes, and I have not diagnosed yet why.
It is a great shame that a correct fix was not as far as I can tell included in the recent 2.2.8-18 from 18 Aug 2016, as I will likely have to wait some time now for a FIX (if my lobbying here succeeds) in 2.2.8-19, unless somebody in authority in this project takes the initiative to fully appreciate that:
The ability to use composite components together with the Facelets caching mechanism is crucial for complex JSF web app development.
It does not currently work properly with 2.2.8-17 (as I've tested, and a quick diff shows no change in 2.2.8-18).
JSF2 users should not have to use JSF2.3 milestones already to get around fundamental problems in 2.2.8-x (and I seemingly can't anyway, lest I spend even more time diagnosing other unrelated problems in JSF2.3.0-m06).
And, as far as reported recently at #4182 (which I can't confirm, because my web app crashes with JSF2.3.0-m06), the facelets caching logic is still not correct in JSF2.3.0-m06.
Therefore, please, as soon as possible, implement finally a fix to the clearly incorrect caching logic in the 2.2.8-x stream.
webelcomau said: I have spawned the request to provide proper fix also for the 2.2.8-1x stream (one that incorporates also the problems identified in #4182 ) as:
@arjantijms said: To state the obvious, minutes of reloading indeed doesn't sound as something we would want. In our own projects like zeef.com, we've never seen that, but then again, we're not using a lot of composite components and favor the lighter weight facelets tags.
The ship has sailed largely, but there seem to be some core issues in composite components that I can't quite put my finger on, but a lot of bugs and weird behavior in the past have been attributed to them.
As for the Facelets refresh, a potential problem is that the code is quite sensitive to changes. What looks like a completely logical change for one bug may cause issues at a totally different place. I'm not too up to date with the details regarding the Facelets cache, but multiple previous Mojarra maintainers told me that they dreaded touching it since every time eventually something else broke.
We currently have very few resources available for Mojarra (and those few resources have to be shared between maintenance and working on 2.3), so that's why this is not getting the attention I agree it should get.
That said, do you have a reproducer page available? Github project would be awesome.
webelcomau said: @arjan tijms Thanks for your reply (and for your other work in Java EE and online Java education).
You wrote:
'The ship has sailed largely, '
It seemed it sailed on 18 Aug 2016 with JSF 2.2.8.18, and the opportunity to fix this was sorely missed. I am imploring you, professionally and personally begging you, in whatever capacity you have to influence this, to lobby for inclusion of a fix in a special JSF 2.2.8.19 release, because this matter has:
Cost me many weeks (actually timed as such in time-tracking software) of unpaid work, and became so bad that I had to completely postpone work on the "culprit" JSF web app project completely for a while (and lesser fans of JSF would have been turned off it forever).
If a fix (in a 2.2.8.19) is not provided for 2.2.8.18, I will have to spend even more time - after the already mentioned many weeks (or even longer I've already spent) wrangling with other issues in 2.3.0-m06 (because it does not work with my main web app anyway for reasons I don't yet understand, but it crashes).
The entire thing ultimately reflects extremely badly on JSF (which is a great shame, because I am a big fan of it, but I can't recommend it to anybody until this caching problem is fixed), and ultimately also on Oracle's seeming lack of recent interest in it. Simple question: can the composite components system for facelets handle a large, complex, professional web app ? Answer: 2016-08-25: No, not yet (and I've worked with it for many years) ! Dr Darren Kelly (Webel IT Australia)
And so, the kiddies run away to the wild-west of PHP + the latest (this week's most-blogged) JavaScript framework: without graphical model-based UML support, or any of the other goodies Java provides.
Besides, if (as reported under #4182) the "FIX" from here does not work for 2.3.0-m06 anyway, the JSF community may as well fix it properly once and for all in both 2.3.0-m06 AND 2.2.8.18, and as soon as possible.
Also, the suggested fix from #4182 (which I can't test) looks simple enough, it's one line.
You asked:
.. do you have a reproducer page available? Github project would be awesome.
I have a screencast that is 2 hours long that demonstrates it happening in both my large real web app and in simple test app (if you reload often enough you can "provoke" it to freeze). It is full of groaning commentary and the sound of me pulling my hair out, but if you provide a way to share it privately I would be willing to provide access to an MP4 of it (you only need to watch a bit to see the problem).
I can't possibly share the real web app that exhibits the problem for commercial reasons (and it needs a commercial ObjectDB licence to run).
I could perhaps prepare a GitHub accessible project that exhibits the problem; it's hard to demonstrate in my current small test web app unless you know how to "provoke" it, but I could do something like include a CC a thousand times to mimic a large web app, maybe that works.
Gees, why not. It's only even more unpaid time spent on one of the most important and basic things JSF has to offer: reloading of composite components for Facelets without redeployment.
A final good word on JSF (why I persist): it is fundamentlally the best designed framework for a comprehensive Expert System web app available. You/we just have to get the #@%^&* Facelets caching to work.
webelcomau said: @arjan tijms
You asked:
.. do you have a reproducer page available? Github project would be awesome.
I answered (partially):
I have a screencast that is 2 hours long that demonstrates it happening in both my large real web app and in simple test app (if you reload often enough you can "provoke" it to freeze). It is full of groaning commentary and the sound of me pulling my hair out, but if you provide a way to share it privately I would be willing to provide access to an MP4 of it (you only need to watch a bit to see the problem).
I have a shorter screencast version (22 mins, but you only need to watch about 13 minutes to see the entire problem demonstrated) from a separate machine (an old MacMini), which I prepared to check it is not just a problem on my main development machine. It also has slightly less groaning and hair pulling.
If you are willing to view it to at least see the problem happening on a large web app, please provide a private channel (via this email form) for me to provide an MP4 download link.
@arjantijms said:
Thanks for your reply (and for your other work in Java EE and online Java education).
Thanks
'The ship has sailed largely,
With that I really meant the design of composite components and/or their core implementation. The idea of them is great, but there's something underneath them that (IMHO) isn't quite right. I've tried to dig through the core composite component code a couple of times to see if I could clearly point out anything that's either wrong or sub-optimal, but it's quite difficult and complicated to grasp.
I can't possibly share the real web app that exhibits the problem for commercial reasons (and it needs a commercial ObjectDB licence to run).
That would absolutely not be needed. We can't ask that in the first place, but even if you would provide that it would be too big and likely have too much other things going on.
What you could do yourself to really increase the chances of getting this fixed is to see if you can isolate the problem to the most minimal application that can possibly exist, preferably using only standard JSF, and in source form (e.g. as a Maven project). If possible, you could also try such minimal example on MyFaces to see if the same thing happens there or not.
I could perhaps prepare a GitHub accessible project that exhibits the problem; it's hard to demonstrate in my current small test web app unless you know how to "provoke" it, but I could do something like include a CC a thousand times to mimic a large web app, maybe that works.
That would be great really. You'd likely want to start by doing just that, and if you can't reproduce it in that way look at what your real app is doing and then try to mimic the kind of components it uses.
For my own reproducers I also take the opposite route occasionally. Start with a copy of my full app, then remove things one by one until the problem no longer occurs, then trace one step back, and then I often have a good base for a minimal reproducer. For instance, this test (which is a form of a minimal reproducer as well) started out as a real app: https://github.com/javaee-samples/javaee7-samples/commit/d25a239cf4054c02afb1fa171b359b0fda46106e
Gees, why not. It's only even more unpaid time spent on one of the most important and basic things JSF has to offer: reloading of composite components for Facelets without redeployment.
Do note that pretty much all Mojarra committers other than the Oracle staff are unpaid as well. As we all have (demanding) day jobs too, this often makes it hard to find some free time or energy in the evenings or during a commute home etc. For now though, I'll try to discuss with BalusC and ZhangXinyuan how to proceed here, and then if the minimal reproducer is there we can hopefully start analyzing what's wrong and if a fix is possible.
webelcomau said: @arjan
Do note that pretty much all Mojarra committers other than the Oracle staff are unpaid as well ...
Oh believe me, I know that already. And it shows too often. What an ad for JSF. It goes something like this:
JSF started as a core component of Sun/Oracle Enterprise Java. Fixes, for Enterprise software are supposed to be tested against Enterprise standard test suites (including composite components), by Enterprise standard software developers (supported). Not just by keen clever people donating their time.
Otherwise Enterprises will rightly conclude that it is does not qualify for Capability Maturity in any form. Which is where it is headed, and unless it is treated more as Enterprise software, it will die, no matter how promising, unless Oracle (an Enterprise) treats it as Enterprise software.
Which is why, although of course I could do it, I am not going to just fiddle with this with my own spin on a FaceletCacheFactory. Because that would not be an Enterprise solution; it would be a band-aid, a workaround.
It needs to be fixed by the Enterprise that launched it. So, is Oracle actually still an Enterprise "behind" JSF ?
webelcomau said:
we all have (demanding) day jobs too
Well, at least that's some time getting basically paid, isn't it. Beats working on Enterprise Java projects not being paid, because one can't bill clients for things that are supposed to work (JSF facelets and composite components) when one has enthusiastically recommended the technology.
@arjantijms said:
It needs to be fixed by the Enterprise that launched it. So, is Oracle actually still an Enterprise "behind" JSF ?
Frankly, we don't know what Oracle's stance is on JSF either. There's some level of support provided by Oracle via Zhijun, ZhangXinyuan and others, and even though I know they are working on it the best they can, it's different from the time when Manfred Riem among others was still working daily on JSF and we saw daily commits.
Coming JavaOne hopefully we'll hear some more. The external committers are as much in the dark about Oracle's plans as everyone else is.
I do think that if you're using WebLogic with a paid support contract you can more directly ask Oracle for fixes. As for GlassFish support, Oracle unfortunately announced a while back they don't commercially support it anymore, but an external company (c2b2/Payara) took up commercial support for that. You could try with them as well.
Well, at least that's some time getting basically paid, isn't it.
Actually, we're hit pretty often by bugs (critical or otherwise) in our day time projects, be it in Hibernate, Weld, the JASPIC implementations of various servers, Postgres, MySQL, Linux even... it's not always possible for us either to bill external customers for hours spend on wrestling with those bugs.
lprimak said: @webelcomau I can understand your frustration. Issues like this are tough to track down. Unfortunately, from my experience, I don't believe that your problem is related to this issue at all. There have been lots of issues with composite components over the years, and I think you are correct that they have not been solved.
Fix by @mmariotti may also be valid, but testing it is a real pain in the rear, as Arjan stated before. That fix, is also unrelated to your problem, I do believe.
One thing you can do is tell us why your app crashes in 2.3.0m06 (in a separate issue) This would be the easiest track to start fixing your other issues, because it is much harder to retrofit all fixes to the 2.2.x tree.
webelcomau said: @lprimak and @arjan tijms.
Thanks both for your constructive replies, and for the time you give to JSF.
@lprimak wrote:
One thing you can do is tell us why your app crashes in 2.3.0m06 (in a separate issue) This would be the easiest track to start fixing your other issues, because it is much harder to retrofit all fixes to the 2.2.x tree.
I have already thought of that, and will invest some time with that immediately first - before trying to create a public test app demonstrating the facelets cache reloading problem in 2.2.8-17, 2.2.8-18, because (reading between the lines in various posting) in the end to benefit from any improvements in the 2.3.0m0x series, where the effort seems to now be going, I am going to have to solve that (and I will report problems encountered in dedicated issue tracking items here). It also means the JSF team can then focus on the suggested fix at JAVASERVERFACES-4178 Facelet cache wrong expiration threshold (2.3.0m06).
@arjan tijms wrote:
As for GlassFish support, Oracle unfortunately announced a while back they don't commercially support it anymore ...
I realised after writing late yesterday evening (from Sydney, Australia) that part of my frustration goes beyond just JSF at the moment. I refrained from mentioning specifics to stay on-topic with this JSF issue (and not to just vent about Java EE in general), but since you raised Glassfish. I am currently stranded on NetBeans81.beta + Glassfish4.1 when working on my main large web app, because whenever I deploy it with NetBeans8.1 + Glassfish4.1.1 it take about 10 minutes just to deploy, instead of about 1 minute ! Now that's truly frustrating; such versions are supposed to get better, not worse.
Add to that endless problems with 3rd party software for Java EE, and the result is that my Java EE development performed for a venture startup has been nearly crippled. I was the lab rat again and again for various confirmed problems encountered in Primefaces (community vs commercial option) ,ObjectDB JPA (commercial only), and JRebel (which does not play nicely with ObjectDB); all of these efforts have definitely matured somewhat in recent years (although new features introduce new problems), but in tackling a very ambitious large web app I think I have unfortunately also too often been the first person it seems to encounter certain inherent problems in these technologies.
I recommended and adopted JSF, JPA, and Java EE in general in the hope that there was a fundamental Capabiity Maturity and robust commercial eco-system; issues like this facelets one reported here as JSF-4107/JSF-4178/JS-4180 and others I mention above for Java EE again and again demonstrate that said Capabiity Maturity is at best fragile. I recommended Java EE some years ago for my main JSF web app project because I evaluated it as lower risk; it has proved anything but.
klavikka said: Btw, I have always thought that expiration based on a refresh interval is quite awkward. A few years ago I wrote a simple replacement for the default facelet cache by using Guava cache for caching and java.nio.file.WatchService for triggering refreshes. I haven't touched the code for quite some time, but so far I haven't had any problems with it.
I some of you find the idea attractive, feel free to give my implementation a try: https://gist.github.com/tuner/1924b56675157a42a259827e97457ce6
@arjantijms said:
A few years ago I wrote a simple replacement for the default facelet cache by using Guava cache for caching and java.nio.file.WatchService for triggering refreshes.
I really like having some existing cache implementation back something like the Facelets cache. Unfortunately, we're very restricted in the external dependencies we are allowed to use in Mojarra. If I understood correctly, if we wanted to introduce an external dependency we'd have to go through an Oracle permission process of some kind.
This is why I've been asking a couple of times for inclusion of JCache in Java EE 8 (as actually was planned).
A ConcurrentMap + java.nio.file.WatchService would by default be okay to use of course.
mmariotti said: @klavikka
very nice idea, but the WatchService could be unusable in some enviroment: url.getProtocol(), i.e. using wildfly, does not always return "file"; depending on deploy strategy and/or facelet position, may return "vfs" - I don't know how (and if it's possible without using wildfly dependencies) to extract a "file" from "vfs".
However, I also think that refresh interval expiration is quite awkward. As for development use, IMHO the best trigger for checking URL modification has to be based on a combination of cache access and lifecycle state (without using a long-life thread).
mmariotti said: Ok, I created a "manual" test app: https://github.com/mmariotti/JAVASERVERFACES-4178 to show the side-effect.
Issue-Links: is cloned by JAVASERVERFACES-4176
This issue was imported from java.net JIRA JAVASERVERFACES-4107
Marked as fixed on Thursday, May 12th 2016, 1:49:11 pm
When looking at expiring Facelets, the code erroneously calls getAndAdd(), thus every time unexpired Facelet is checked in the cache, the TTL will be bumped up another REFRESH_INTERVAL. The effect of this is that if you have FACELET_REFRESH_INTERVAL of (for example) 5 minutes, and within that 5 minutes the Facelet gets touched, the TTL will be pushed back another 5 minutes, thus the cache will never expire, when it should
DefaultFaceletCache.java:219
and getNextRefreshTime() :
Environment
all
Affected Versions
[2.2.8-12, 2.3.0-m04, 2.3.0-m05]