StackOverflowError from org.apache.wicket.Session when detaching/serializing page #21
Comments
|
Not good. The above only tells me there's some problem with Apache Wicket (which is used by Gitblit) trying to serialize some page. With only that, I have no chance of tracking this down. Can you give me more clues? Is there anything more in the log? Any hint which page it might have been? Anything in the log just before or after? Any clues from Gerrit's httpd log about the actions that occurred at about the same time? And -- does it also occur with the latest version? |
|
Just upgraded to Gerrit version 2.12.2 and installed gitblit plugin version 2.12.171.1 and see same Exception: [2016-07-15 20:19:39,068] [HTTP-582] ERROR org.apache.wicket.Session : Exception when detaching/serializing page |
|
So it occurs also with the latest version. But that stack trace still gives no hint at what the source of the problem might be. See my questions above. And one additional question: what the symptoms on the UI? |
|
Same over here but just with the really big repositories, larger than 1 Gb and 60,000 commits. |
|
This is truly annoying. Any hint which page it might be? |
|
Took a long look at the Gitblit internals for the umpteenth time. I don't see anything obvious, though I do see some suspicious things:
What causes trouble for Java serialization are long chains of object references. (Circular references should be no problem.) RevCommit in particular has links to its parent commits. I'm not sure where the actual bug is now: JGit or Gitblit? Should the parent links in a JGit RevCommit even be serialized, or should they be transient? (There is one more link in a JGit RevCommit that definitely should not be serialized; it's an internal hash collision chain link.) In any case, Gitblit should take care not to try to serialize JGit objects when its pages get serialized. Fixing that is not going to be easy. Note that all this is so far a conjecture only, but it could fit the stack traces shown here: RevCommit has the parent links as "RevCommit[] parents" (zero, one, or two), which would match the java.io.ObjectOutputStream.writeArray invocations shown in the traces. For now, try switching off that activity panel. Set |
Two Wicket pages in Gitblit had non-transient loggers. Replace them by transient ones.
JGit RevCommits contain parent links, which can lead to deep recusions during Wicket's page serialization, until a StackOverflowError occurs. Avoid this by making sure that Wicket objects (pages, panels, or also models) do not use RevCommit directly but always the Gitblit-own RepositoryCommit wrapper. Give RepositoryCommit its own custom serialization which does not write the RevCommit but only its SHA-1 and upon reading re-fetches the commit from the repository. Also ensure that Gitblit's RefModel does not serialize RevObjects. Adapt callers, where necessary. Fix occurrences of Wicket DataViews instantiated with RevCommit. Make those use RepositoryCommit, too.
|
Indeed the following snippet reproduces a stack trace very similar to the ones shown here: Serializing the first commit will in fact attempt to recursively serialize all commits read. Ka-boom. (When exactly it crashes depends on the stack size; for me it crashes on the gitblit repo just short of 2000 commits.) Gitblit does similar things in a few places, and even though it does in general pay attention not to load thousands of commits, it may well be that it ends up with similar parent commit chains. I've put together a draft release of the plugin that works around this issue and that tries to ensure that Wicket pages never try to serialize JGit RevCommits. Please give v2.12.171.4 a try, and please do report back if that helped. It's a bit a shot in the dark as I have never experienced this StackOverflowError myself. It this does solve the problem, I'll contribute a patch upstream to gitblit. |
|
Thank you for the update. I just tried and get a Server Error when using it on the same repository. At least the previous one it shown the error but it shown the data in the GUI, this one shows "Server Error": [2016-10-25 08:16:06,409] [HTTP-37] WARN org.eclipse.jetty.servlet.ServletHandler : Error for /plugins/gitblit/summary/;jsessionid=eqck15d7r2h6314pa636or8h |
|
What an embarrassing and stupid bug of my part. It's a typo. Don't know how that slipped in, and why I didn't see it in my tests. :-( Don't have time right now, but I'll fix that tonight. |
|
Should be fixed now. If you re-download v2.12.171.4 again and install it in Gerrit, this NoSuchMethodError should no longer occur. Note that you have to re-download; I overwrote the previous broken v2.12.171.4. |
|
Downloaded and tried. Same error than the original one. Also the gitblit.properties used is as follows: |
In other words, StackOverflowError? |
|
Yes |
|
The project has 15262 changes/patch sets. |
|
With my change no JGit RevCommits should be serialized anymore. Either it's not that, or I missed some somewhere... I'll take another look. |
|
Don't see anything. I'm out of ideas. Note that this StackOverflowError has also been reported against plain Gitblit: gitblit-org/gitblit#1011, and over at repo-discuss also against the official plugin. Also Gerrit bug 3316 and this comment. Also this really old report against Gitblit 1.3.2/1.4.0. I can try to add more logging to try to figure out where this is coming from... |
|
Will help in any way raising the level of Gerrit logging ? |
Might be worth a try. But frankly said, I don't know where Gitblit/log4j picks up the logging config when run as a Gerrit plugin. |
|
After some more research, I think I've found another problem spot: Gitblit uses in some cases anonymous inner classes that get serialized. That will also serialize some synthetic fields created to access final variables from outside. Such is the case for instance in HistoryPanel, which I thought I had fixed. Alas, the Wicket DataView used (line 155) will contain a synthetic field to the final variable Not sure I can fix this in all cases in the plugin; depends on where else this code pattern occurs. The few cases directly related to JGit objects that I've found so far I can deal with; but I don't know what I'll find in other page/panel classes. But in any case I'll give it another try. BTW, serializable inner classes are discouraged and what I see in HistoryPanel looks very similar to well-known anti-patterns for Wicket applications. :-( |
Gitblit uses a few inner anonymous classes in some pages. Ensure that those do not hold references to JGit RevCommits to avoid that they get serialized when the page is. The usage of serialized inner anonymous classes is an anti-pattern in Wicket programming and is discouraged in the Java serialization spec, but cleaning this up properly would amount to a larger refactoring than can be done in this plugin (and also larger than I'm willing to do).
|
Let's try again:
So if my conjecture that the StackOverflowError comes from inadvertently serializing JGit RevCommit objects with long parent chains is correct, we stand a fair chance that v2.12.171.5 doesn't have that bug anymore. Could you try again, please? I pulled the v2.12.171.4 release. It had already been downloaded 6 times, so I didn't want to overwrite it again. |
|
Great Tomas. I think you are right in the new findings. It is working OK now. Maybe you should spread this change as well for the 2.13 plugin. And maybe let the original GitBlit know about it. |
JGit commit objects are a recursive data structure; they have links to their parent commits. Serializing a JGit commit will try to recursively serialize all reachable ancestors as faras they have been loaded. If that ancestor chain is too long, a StackOverflowError is thrown during Wicket's page serialization if a page has a reference to sucha JGit commit. Fixed by making sure that pages o not contain references to JGit commits. Use the (existing) wrapper object RepositoryCommit instead. * RepositoryCommit has a transient reference to the JGit commit and reads the commit from the repository upon de-serialization. * RefModel is a similar case (JGit tags/branches may also have links to the commits they point to). Solved a bit differently by making it a pure data object by transferring the interesting data from the JGit object in the constructor. * Change DataViews instantiated with RevCommit to use RepositoryCommit instead. * Change inner anonymous DataViews to ensure they do not have a synthesized field referencing the "allRefs" map. Such a synthesized field would also get serialized, and then serialize JGit commits again. Finally, remove non-transient logger instances in Wicket classes. Those might lead to NotSerializableException. These StackOverflowErrors have been reported in several places since 2014: * https://groups.google.com/forum/#!topic/gitblit/GH1d8WSlR6Q * https://bugs.chromium.org/p/gerrit/issues/detail?id=3316 * https://groups.google.com/d/msg/repo-discuss/Kcl0JIGNiGk/0DjH4mO8hA8J * https://groups.google.com/d/msg/repo-discuss/0_P6A3fjTec/2kcpVPIUAQAJ * gitblit-org#1011 * tomaswolf/gerrit-gitblit-plugin#21
|
Plugin release v2.13.171.1 is published. Pull request for upstream Gitblit is gitblit-org/gitblit#1141. Thanks a lot for testing! |
JGit commit objects are a recursive data structure; they have links to their parent commits. Serializing a JGit commit will try to recursively serialize all reachable ancestors as faras they have been loaded. If that ancestor chain is too long, a StackOverflowError is thrown during Wicket's page serialization if a page has a reference to sucha JGit commit. Fixed by making sure that pages o not contain references to JGit commits. Use the (existing) wrapper object RepositoryCommit instead. * RepositoryCommit has a transient reference to the JGit commit and reads the commit from the repository upon de-serialization. * RefModel is a similar case (JGit tags/branches may also have links to the commits they point to). Solved a bit differently by making it a pure data object by transferring the interesting data from the JGit object in the constructor. * Change DataViews instantiated with RevCommit to use RepositoryCommit instead. * Change inner anonymous DataViews to ensure they do not have a synthesized field referencing the "allRefs" map. Such a synthesized field would also get serialized, and then serialize JGit commits again. Finally, remove non-transient logger instances in Wicket classes. Those might lead to NotSerializableException. These StackOverflowErrors have been reported in several places since 2014: * https://groups.google.com/forum/#!topic/gitblit/GH1d8WSlR6Q * https://bugs.chromium.org/p/gerrit/issues/detail?id=3316 * https://groups.google.com/d/msg/repo-discuss/Kcl0JIGNiGk/0DjH4mO8hA8J * https://groups.google.com/d/msg/repo-discuss/0_P6A3fjTec/2kcpVPIUAQAJ * gitblit-org#1011 * tomaswolf/gerrit-gitblit-plugin#21 Issue: gitblit-org#1011 (cherry picked from commit 005e8e2)
JGit commit objects are a recursive data structure; they have links to their parent commits. Serializing a JGit commit will try to recursively serialize all reachable ancestors as faras they have been loaded. If that ancestor chain is too long, a StackOverflowError is thrown during Wicket's page serialization if a page has a reference to sucha JGit commit. Fixed by making sure that pages o not contain references to JGit commits. Use the (existing) wrapper object RepositoryCommit instead. * RepositoryCommit has a transient reference to the JGit commit and reads the commit from the repository upon de-serialization. * RefModel is a similar case (JGit tags/branches may also have links to the commits they point to). Solved a bit differently by making it a pure data object by transferring the interesting data from the JGit object in the constructor. * Change DataViews instantiated with RevCommit to use RepositoryCommit instead. * Change inner anonymous DataViews to ensure they do not have a synthesized field referencing the "allRefs" map. Such a synthesized field would also get serialized, and then serialize JGit commits again. Finally, remove non-transient logger instances in Wicket classes. Those might lead to NotSerializableException. These StackOverflowErrors have been reported in several places since 2014: * https://groups.google.com/forum/#!topic/gitblit/GH1d8WSlR6Q * https://bugs.chromium.org/p/gerrit/issues/detail?id=3316 * https://groups.google.com/d/msg/repo-discuss/Kcl0JIGNiGk/0DjH4mO8hA8J * https://groups.google.com/d/msg/repo-discuss/0_P6A3fjTec/2kcpVPIUAQAJ * gitblit-org#1011 * tomaswolf/gerrit-gitblit-plugin#21 Issue: gitblit-org#1011 (cherry picked from commit 005e8e2)
JGit commit objects are a recursive data structure; they have links to their parent commits. Serializing a JGit commit will try to recursively serialize all reachable ancestors as faras they have been loaded. If that ancestor chain is too long, a StackOverflowError is thrown during Wicket's page serialization if a page has a reference to sucha JGit commit. Fixed by making sure that pages o not contain references to JGit commits. Use the (existing) wrapper object RepositoryCommit instead. * RepositoryCommit has a transient reference to the JGit commit and reads the commit from the repository upon de-serialization. * RefModel is a similar case (JGit tags/branches may also have links to the commits they point to). Solved a bit differently by making it a pure data object by transferring the interesting data from the JGit object in the constructor. * Change DataViews instantiated with RevCommit to use RepositoryCommit instead. * Change inner anonymous DataViews to ensure they do not have a synthesized field referencing the "allRefs" map. Such a synthesized field would also get serialized, and then serialize JGit commits again. Finally, remove non-transient logger instances in Wicket classes. Those might lead to NotSerializableException. These StackOverflowErrors have been reported in several places since 2014: * https://groups.google.com/forum/#!topic/gitblit/GH1d8WSlR6Q * https://bugs.chromium.org/p/gerrit/issues/detail?id=3316 * https://groups.google.com/d/msg/repo-discuss/Kcl0JIGNiGk/0DjH4mO8hA8J * https://groups.google.com/d/msg/repo-discuss/0_P6A3fjTec/2kcpVPIUAQAJ * #1011 * tomaswolf/gerrit-gitblit-plugin#21
Gerrit 2.12.2 with GitBlit 2.12.162.1
We regularly see Gerrit's
error_logflooded withThe text was updated successfully, but these errors were encountered: