Closed bgooren closed 5 years ago
Here are the options that I see:
1) do not keep a reference to WebjarsJavaScriptResourceReference
2) use static
member fields for WebjarsJavaScriptResourceReference
3) fallback to WebjarsSettings
at https://github.com/l0rdn1kk0n/wicket-webjars/blob/d96205150f796f615b1509196df91111a6168c9e/library/src/main/java/de/agilecoders/wicket/webjars/WicketWebjars.java#L138 instead of throwing an exception but then it will break silently any apps which use custom settings
I'll have a deeper look tonight but so far I don't see a good solution
Can you prepare a small quickstart app to play with ? I think I can save and restore the application during deserialization of WebjarsJavaScriptResourceReference and later it should be available at WicketWebjars#settings().
Hi Martin,
I'll make a quickstart as soon as possible and will provide it here when done :-)
Ok - quickstart attached. quickstart.zip
Reproducing the bug is easy:
This gives me the same (inner) exception:
Caused by: java.lang.IllegalStateException: there is no active application assigned to this thread.
at de.agilecoders.wicket.webjars.WicketWebjars.settings(WicketWebjars.java:138)
at de.agilecoders.wicket.webjars.util.WebjarsVersion$Holder.<clinit>(WebjarsVersion.java:29)
Thanks!
Hi @bgooren !
It's been a while since you reported this issue. I've just tried to reproduce it but it seems Jetty doesn't store the sessions. I don't see anything about this neither in Start.java nor in jetty.xml files. Do you recall how you started Jetty - with Start.java or by using external Jetty instance ?
Hi @martin-g,
I'll try to reproduce it - I'll need to check the quickstart myself as it's been a while since I reported this :-)
Hi @martin-g,
It took longer than expected to find some time to look at this. I start the jetty instance using
mvn jetty:run
(the jetty session storage is configured in pom.xml)
It took two tries (start jetty, open http://localhost:8080, click link a couple of times, stop jetty, start jetty again), but I was able to reproduce. See attached startup log. startup.log
As I am interested in serialization issues, I take some time to see what happens. From my point of view:
In a running application, serialization/deserialization of session is generally triggered by wicket, and done a wicket-aware thread. So this bug is only triggered at application startup.
Good analysis, @lalmeras ! I knew why it is broken, I just needed a demo application so I can test whether a potential fix actually works. Do you want to try to fix it as I suggested at https://github.com/l0rdn1kk0n/wicket-webjars/issues/51#issuecomment-271931153 ? My idea is to override writeObject() and to write the application name in addition to the default fields. Then in readObject() to use the application name and export the Application ThreadLocal (see ThreadContext.setApplication())
Hm. But this will leak the thread local I am afraid.
I don't think this is the right approach to fix this. IMO static initializers should not call any statefull code (like WebjarsSettings.get()...)
Thinking about a way to fix it, I try to find a similar usecase in Wicket, and it seems to me that there is nowhere in wicket code something like static Application staticField = Application.get() ; and that is on purpose.
So I propose to move all Holder attributes to Holder methods (that in turn call WicketWebjars.settings()) in the first place. This modification would allow to use Webjars*ResourceReference as a non-static field value and only involve modifying private API.
This is not enough to use it as a WebjarsResourceReference as it implies to call useRecent(...) and stateful code at initialization time. But as declaring ResourceReference as static field is a common pattern with wicket, I think as necessary to find a way to allow stateless initialization (without useCurrent(...) call) of WebjarsResourceReference.
And tricking serialization is useless I think, as the problem is part of static code (no ser/deser invoked here).
Any feedback on my proposal ? Do you think my analysis is right ? Do you agree on the proposed modifications (get rid of attributes in Holder, get rid of useRecent() call in initialization code).
Are you interested by a PR, or do you prefer to work on your own fix ?
@martin-g Any insight on my last feedback ?
@lalmeras Please create a PR!
Working on it ; code is already there : https://github.com/igloo-project/wicket-webjars/tree/issue-51-deserialization
I still need to reproduce the issue with the quickstart.zip to ensure my fix is working before creating a PR.
I work again on this, and slightly change what I done before.
Previous branch https://github.com/igloo-project/wicket-webjars/tree/issue-51-deserialization is moved to https://github.com/igloo-project/wicket-webjars/tree/issue-51-deserialization-holder : it does not fix the issue for quickstart.zip, as it uses Webjars...ResourceReference in a static attribute, so calls useRecent(...) at attribute deserialization time, so it still triggers Holder.* methods at a time they cannot work.
I push a new branch https://github.com/igloo-project/wicket-webjars/tree/issue-51-deserialization-useRecent that moves useRecent(...) stateful method at resource delivery time. It adds computations on each resource dellivery (useRecent is now called by WebjarsResourceFinder#find) , but it allows to use static attributes for Webjars*ResourceReference and to deserialize components at page deserialization time.
(I work on wicket 8.x branch, so I use quickstart.zip with modified dependencies to wicket 8.x and wicket-webjars 2.x)
Hi! Just wanted to check if this can be closed, since the PR with a fix was merged, correct?
No. The PR introduced problems. There is a new PR which I have to take a deeper look at...
On Wed, Feb 13, 2019, 18:46 Sebastian Gooren <notifications@github.com wrote:
Hi! Just wanted to check if this can be closed, since the PR with a fix was merged, correct?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/l0rdn1kk0n/wicket-webjars/issues/51#issuecomment-463274207, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOKQnDyGKS6rEKMMTQi7xxQfD5ZP61fks5vNEFdgaJpZM4LgvtY .
Closing this issue! Please test it when you have a chance with 2.0.11
Hi!
I'm testing with wicket 8.4.0 and wicket-webjars 2.0.11, and still see the error sometimes:
apr. 09, 2019 4:59:36 P.M. org.apache.catalina.session.StandardManager startInternal
SEVERE: Exception loading sessions from persistent storage
java.lang.ExceptionInInitializerError
at de.agilecoders.wicket.webjars.util.WebjarsVersion.useRecent(WebjarsVersion.java:45)
at de.agilecoders.wicket.webjars.request.resource.WebjarsJavaScriptResourceReference.<init>(WebjarsJavaScriptResourceReference.java:29)
at com.ffour.lib.storage.upload.dropzone.DropzoneBehavior.<clinit>(DropzoneBehavior.java:41)
... snip ...
at org.apache.catalina.startup.Bootstrap.main(Bootstrap.java:492)
Caused by: java.lang.IllegalStateException: there is no active application assigned to this thread.
at de.agilecoders.wicket.webjars.WicketWebjars.settings(WicketWebjars.java:141)
at de.agilecoders.wicket.webjars.util.WebjarsVersion$Holder.<clinit>(WebjarsVersion.java:29)
In DropzoneBehavior
we have static webjars resource references:
public abstract class DropzoneBehavior extends AbstractDefaultAjaxBehavior
{
public static ResourceReference JS = new WebjarsJavaScriptResourceReference( "dropzone/current/dropzone.js" );
public static ResourceReference CSS = new WebjarsCssResourceReference( "dropzone/current/dropzone.css" );
...
It looks like something went wrong while merging the changes in #57 into the wicket-8.x branch...
See https://github.com/l0rdn1kk0n/wicket-webjars/commit/6e0e82f994b023c10290f04138efadebef208ffb
In wicket-webjars 2.0.11 the call to useRecent
is still in the constructors of WebjarsJavaScriptResourceReference
and its CSS variation.
This is from the file I downloaded from maven central (2.0.11):
public WebjarsJavaScriptResourceReference(final String name) {
super(WebjarsJavaScriptResourceReference.class, useRecent(prependWebjarsPathIfMissing(name)));
this.originalName = name;
}
Can you please double-check? Perhaps I am missing something...
I confirm it seems to be an issue with #58 merge (6e0e82f994b023c10290f04138efadebef208ffb is the merge of #58, not #57). useRecent calls should not be there.
Without useRecent()
the produced url would be .../current/.../some.css
instead of .../1.2.3/.../some.css
and Wicket-Webjars won't do any help.
So, yes. It seems this issue is not resolved!
Added a workaround that uses the default WebjarsSettings when there is no Application thread local. 2.0.12 is on its way to Maven Central
Hi!
During development (on local osx machine, running eclipse; the app runs inside tomcat 8, managed from within eclipse) we are having issues when deserializing wicket sessions on application (re-) start. Deserialization fails on WebjarsJavaScriptResourceReference.
Should we not keep a reference to WebjarsJavaScriptResourceReference in our components?
Stack trace: