« Setup Apache 2 and... | Main | One thing's for sure »
20040819 Thursday August 19, 2004

FindBugs I ran FindBugs on AppFuse last night and found/fixed a number of issues as a result. I'm now down to only a handful left - most of them being "Class is Serializable, but doesn't define serialVersionUID". I tried to generate one using serialver, but I couldn't it to work after numerous attempts. My issues seemed to be classpath related: it wanted the servlet api in my classpath, and once I'd add that, it could find my class. I'll have to try the SerialVer Ant Tasks. Fixing this issue would be nice, but I doubt it's really affecting appfuse-based applications. The other bug is "Inconsistent synchronization" in UserCounterListener.contextInitialized() method. Any tips on solving this one are appreciated. Posted in Java at Aug 19 2004, 09:46:50 AM MDT 15 Comments

Comments:

> The other bug is "Inconsistent synchronization" in
> UserCounterListener.contextInitialized() method.
> Any tips on solving this one are appreciated.

I guess it's because the methods contextInitialized & contextDestroyed are not synchronized and they use "users" and "counter", while other synchronized methods also use them.

Posted by Guillaume Poirier on August 19, 2004 at 10:15 AM MDT #

Yeah, I figured that much out. I guess the question is whether these methods really need to be synchronized?

Posted by Matt Raible on August 19, 2004 at 10:24 AM MDT #

This article might help: http://www-106.ibm.com/developerworks/library/j-jtp06294.html?Open&ca=daw-ja-news Specifically the "Being nice to the memory model" section: In "Fixing the Java Memory Model, Part 1," I reviewed the basic rule for synchronization -- whenever reading a variable that might have last been written by another thread, or writing a variable that might next be read by another thread, you must synchronize. It is easy to "forget" this rule, especially when reading -- but doing so creates numerous risks for the thread-safety of your program. This sort of bug is commonly introduced when maintaining a class that was originally properly synchronized, but the thread-safety requirements were not fully understood by the maintainer. "Fortunately, FindBugs has a number of detectors that can help identify incorrectly synchronized classes. The Inconsistent Synchronization detector is probably the most complicated detector used by FindBugs; it has to analyze the whole program, not just individual methods, use dataflow analysis to determine when a lock is held, and use heuristics to infer that a class intends to provide thread-safety guarantees. Basically, for each field, it looks at the pattern of accesses for that field, and if the majority of accesses are done with synchronization, accesses without synchronization are flagged as potential errors. Similarly, the Inconsistent Synchronization detector will issue a warning if the setter for a property is synchronized and the getter is not. In addition to inconsistent synchronization, a number of other detectors for common threading errors are included, such as waiting on a monitor with two locks held (which, while not necessarily a bug, could cause a deadlock), using the double-checked locking idiom, incorrect lazy initialization of nonvolatile fields, invoking run() on a thread instead of starting it, invoking Thread.start() from a constructor, or calling wait() without wrapping it in a loop. "

Posted by dsuspense on August 19, 2004 at 11:12 AM MDT #

It is because the servletContext.setAttribute() method call is not synchronized, due to the contextInitialized() method not being synchronized . All other calls to servletContext.setAttribute() everywhere else in the UserCounterListener class are called from synchronized methods.

Posted by dsuspense on August 19, 2004 at 11:17 AM MDT #

I thought for sure I tried adding "public synchronized void" to those methods last night and it didn't work. Oh well, musta been late - works now! Thanks guys.

Posted by Matt Raible on August 19, 2004 at 11:57 AM MDT #

Matt, did you find using FindBugs very helpful? I've never used any static analysis tools before but it seems like a very interesting idea. Anyone else familiar with FindBugs or packages like PMD?

Posted by Keller on August 19, 2004 at 12:51 PM MDT #

More than anything, I think if your application works and all your tests pass - then that's the best indicator of quality code. However, these tools do help and show you things that you might not realize. Thanks for the PMD reminder, that's integrated into AppFuse and I just cleaned up a couple more classes using it.

Posted by Matt Raible on August 19, 2004 at 01:27 PM MDT #

I think Findbugs is definitely worth using. It can catch things like improper access levels, unclosed resources (i.e. streams), threading problems, etc. I wouldn't run it every time I do a build though - it takes several minutes to run on a small code base. But maybe once a day or once a week depending on how often the code base changes.

Posted by Gary Blomquist on August 19, 2004 at 02:25 PM MDT #

Hey Matt, Eclipse 3.1 M1 has some new quick fixes for serial ids: "Two new Java quick fixes have been added to generate serial version IDs for classes implementing java.io.Serializable." You can find out more about it in the new and noteworthy page.

Posted by Vinu on August 19, 2004 at 02:59 PM MDT #

private static final long serialVersionUID = <anyLongValueYouPleaseHere>;

Posted by Santa Claus on August 20, 2004 at 01:07 AM MDT #

Oops, that should've read private static final long serialVersionUID = any-long-value-here;

Posted by Santa Claus on August 20, 2004 at 01:07 AM MDT #

!Vinu - thanks for the tip. I downloaded and installed 3.1 M1 and found this option. However, when I say "generate a serialVersionUID for the class", it generates the following: [{Java2HtmlPlugin private static final long serialVersionUID = 1L; }] I was expecting some randomly generated number, rather than the same number for each class. Should I use "1L" or is ther a bug in this Eclipse feature?

Posted by Matt Raible on August 21, 2004 at 10:16 PM MDT #

Actually, serialver.exe does not generate a random number, it will always generate the same number for two classes that match the criterias used by their algo. Those criterias are things like the class name, variables names and types, and the method signatures. It basically generates the same number that would be generated at runtime if none is specified in the class. What number you want there really depends on how you want to handle the possibily that an object of that class is serialized, then deserialized with a different version of the same class. If don't define any serialVersionuID, then any change to the class will have the deserialization fails, which might or might not be what you want, depending on your use case. If you set it to always the same number, then any change won't prevent the deserialization, but if your changes were not compatible, then you might get a corrupted object. But I took a quit look at AppFuse, and you don't seem to use Serliazation anywhere, do you? The only place I can think of where it would make any difference is if Tomcat serialize the content of the HttpSession. And in such case, I assume you would want to deserialization to fail when in doubt, thus not specifiying any serialVersionUID. But like I said, it depends on your particular situation.

Posted by Guillaume Poirier on August 22, 2004 at 08:56 AM MDT #

<em>> But I took a quit look at AppFuse, and you don't seem to use Serialization anywhere, do you?</em>

You are correct - I don't really need it - that's why I'm wondering if I should add them. FindBugs says you're supposed to add them to any class that implements Serializable. This is basically any Servlets, Taglibs or model objects. If it won't hurt, I'll add them - but I want to make sure "1L" is the proper value to assign. I'm basically just trying to get a bug-free app (according to FindBugs).

Posted by Matt Raible on August 22, 2004 at 09:44 AM MDT #

Well, like I said, I find that there are situations where it would be valid not to define any serialVersionUID, so I would question the relevance of such warning. But if you just want to get rid of the warning, and can't disable it, then I can't see any reason where the actual value would matter for you, except may be for the domain objects, if they goes in an HttpSession. That is because Tomcat, depending on your configuration, might serialize the content of the HttpSession on server shutdown, and load the session content back again on server startup. In developpement environnement, if your domain object changed, you probably want the deserialization to fail, because it could lead to unexpected exceptions. Then there's also the clustering issue, but I guess in your case you probably don't care about it.

Posted by Guillaume Poirier on August 22, 2004 at 10:14 AM MDT #

Post a Comment:
Comments are closed for this entry.
Click me to subscribe
Matt Raible is a Web Architect who enjoys developing applications with open source technologies. Contact me for rates.
« November 2008
SunMonTueWedThuFriSat
      
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
      
Today

Recent Entries

Tag Cloud

abbie acegi appfuse denver flex grails gwt hibernate jack java jsf linkedin maven myfaces oscon rails skiing spring springmvc struts struts2 tapestry travel webframeworks wicket