catching bugs while building elements

I’m looking at Builder.create() right now, trying to determine whether/how
to refine what the method throws. Presently, it throws Exception, which is
both too much and too little. It’s too much, because this means that we’re
forced to catch RuntimeExceptions somewhere above Builder.create() in the
call stack; too little, because there are some descendants of Error thrown
here which normally should be propagated all the way up to the default
handler—except in this case, where we’re loading classes by reflection,
and so we really want to catch those.

So, here’s my question:

If we’re trying to load a class which is part of VASSAL and that fails,
then that’s clearly a bug and should result in the bug reporting dialog
being displayed. However, if we’re trying to load a custom class which
is part of a module and that fails, it’s not clear to me what we should
do. In that case, do we really want to receive bug reports?

On a related note, I’ve set up a test Bugzilla instance at nomic.net, and
I’m pretty well convinced now that we’ll be much happier using Bugzilla
than SourceForge’s bug tracker. That raises two issues:

  1. Where do we want it to be hosted? I’m willing to host it at nomic.net,
    but it would be better to have it at vassalengine.org. (Is that possible?
    If it’s not, then I have some thoughts about hosting the whole domain
    elsewhere, as I’d also like to get MediaWiki setup at vassalengine.org,
    and by the end of summer we’ll have a dice server which we might also want
    hosted there. This all points to hosting somewhere where we have root on
    our own box.)

  2. Bugzilla makes it easy to report bugs on different “products”. This is
    relevant to what we should do when loading a custom class fails, as it
    would be possible to automatically log bugs against individual modules in
    Bugzilla. This would eliminate the problem of mixing module-specific bugs
    in with general VASSAL bugs…

Thoughts?

Joel,

Having bugzilla tracking for each module would be extremely useful for people like me who have 10+ active modules out there. I would be all for it.

In my experience though, custom code generally only breaks at build time when I am developing and testing. In real life, custom code nearly always throws an exception after build when the component starts being used.

Can the bug handler identify the source of a run-time (as opposed to build time) exception and determine if it is outside of the standard Vassal class tree?

B.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Brent Easton”:

Ok, good. I was thinking this would be useful for tracking non-technical
module bugs as well (e.g., 1/507/101 should be 3-5-4, not 3-4-4).

This is pretty easy to handle, I think—it amounts to looking in the
stack trace and blaming the deepest class which belongs to VASSAL or
the module.

Doing what I described above at run-time might be less reliable than at
doing it build-time. It’s plausible that we could have a custom class
which exposes a VASSAL bug, but the exception is thrown from the custom
clas. My method would blame the custom class for the breakage, which isn’t
right.


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

Yes indeed, not to mention the list of enhancements that sits on little scraps of paper for months on end :slight_smile:

I don’t think this would be too serious a problem. If the bug only appears due to Custom code, then the custom code author needs to check it out carefully before passing it on to the Vassal developers.

What would be the ‘product code’ for Bugzilla submissions? Perhaps a ‘Bugzilla Key’ in the module metadata rather than using the internal module name? Then a developer could manage a series of related modules as one Bugzilla product. (Or keep 2 versions together if the internal module name is changed). I am not familiar with BugZilla. Does a ‘product’ have to be set up in advance, or is it just a free-form text field that can take any value?

B.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Brent Easton”:

There are two fields in Bugzilla which are relevant here: Product and
Component. There’s no requirement to use them in any particular way, except
that the interface suggests that Components are parts of Products. E.g.,
Fedora’s Bugzilla has Fedora (and other things, like RHEL) as the Product
and the packages as the Components. Gnome’s Bugzilla has parts of Gnome
(like GConf and evince) as the Products and and aspects of the Gnome apps
(docs, performance, API) as the Components. Mozilla.org’s Bugzilla is
like this as well.

When a Fedora package name changes, there’s nothing in Bugzilla to link the
old package with the new package, but I’ve never noticed it to be a problem.

Products and Components do need to be setup by an admin, but it’s easy
to do (you can set up a Product or Component in about 30 seconds). It’s
also easy to reassign bugs to different Products or Components. (You
select a different one fm the list, and submit.)

You can play with my test setup here: bugs.nomic.net/

Don’t worry about messing it up—regardless of what we do or where
it’s ultimately hosted, I’m going to blow away this test instance after
setting up the real one.

I was thinking that VASSAL (and VASL) should be Products, so that we
can use Component to categorize the bugs. For modules, we could make
each game a Product, and then each module for it a Component, or we
could have each module be a Product.


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

If you can install it via http/ftp, I can set up an ftp account and subdomain for it, like the forums, wiki, and tournaments. Same for MediaWiki.

rk

Post generated using Mail2Forum (mail2forum.com)

I’ve realized now that I have a second question related to (or rather,
antecedent to) the discussion here. There are a number of places where
we’re catching Exception or Throwable. Some of these are clearly
overbroad (like catching Exception from a try block where the only thing
which could be thrown is a NumberFormatException) and I’ve already dealt
with them. For the ones which are left, it’s not so obvious to me what
to do with them.

Two examples:

Command.execute() has this bit of code in it for executing subcommands:

  for (Command c : seq) {
    try {
      c.execute();
    }
    catch (Exception ex) {
      // ... do something to handle it
    }
  }

Now, execute() isn’t declared to throw any checked exceptions, so it’s only
RuntimeExceptions which could ever be caught here. So far as I can see, any
RuntimeException which sneaks out is due to a bug somewhere in the code
called when the subcommand is executed. Is the reason for wanting to catch
RuntimeExceptions here because we want VASSAL to degrade gracefully when a
Command fails? If so, then there might be some descendants of Error which
we should also be catching here so that they aren’t kicked upstairs.

There’s a place in Builder where we catch Throwable. This strikes me as
wrong, since it catches at least one Error (OutOfMemoryError) which
shouldn’t be handled like the others and should go all the way to the top.
Probably there are some other Errors which indicate an utterly
unrecoverable state and should also not be caught here (VirtualMachineError
and its descendants) as we simply won’t be able to soldier on after they
occur. But other Errors might be local to the class being built, e.g., a
custom class could throw an InstantiationError without jeopardizing the
ability of VASSAL to keep running, and so we should probably catch those
here.

I’d like to hear what you guys think about how these two cases should be
handled. Together they comprise almost all of the error handling work I
have left.

Joel,

I defer to your good judgement on this. The plan you have outlined above sounds fine to me.

Brent.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Brent Easton”:

That’s exactly the response I was hoping not to get, unfortunately. My
good judgment isn’t providing me with an answer in this case. What I’m
stuck on are the details, i.e., which Errors and Exceptions should/should
not be caught in these two cases.


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

To tell you the truth, error handling is one of the weakest points of code I write. I’m not sure I even qualify to make an intelligent answer, I could barely follow your arguments. I will try and make sense of it make some comments.

B.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Michael Kiefte”:

Yes, but I suspect that there are a large number of wrong answers.


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Michael Kiefte”:

Don’t quit your day job. :stuck_out_tongue:

Here’s what’s in the try block of Buildable.create(Element) right
now in my working copy:

final Class<?> c = mod == null ?
Class.forName(e.getTagName()) :
mod.getDataArchive().loadClass(e.getTagName());

b = (Buildable) c.newInstance();

Class.forName() can throw LinkageError, ExceptionInInitializerError,
and ClassNotFoundException, and I believe DataArchive.loadClass()
will throw these as well. None of these are recoverable: LinkageError
probably means that the class we were trying to load needs recompiling,
ExceptionInInitializerError probably means that there’s a bug in a
static initalizer (though this wraps another exception, and so we should
probably look at getCause() to see what the cause was), and we can’t
load a class we can’t find. I think that’s exhaustive for these three
lines of code, since any exception thrown as a result of a static
block in the class being loaded will be wrapped in an
ExceptionInInitializerError.

These three all indicate serious bugs if they happen with a VASSAL
class (if we can’t load Map, we’re toast, for example), but are maybe
not unrecoverable if they happen for a custom class.

The cast in the fourth line can throw ClassCastException if the class
doesn’t implement Buildable, and newInstance() can throw
IllegalAccessException, InstantiationException,
ExceptionInInitializerError, and SecurityException. I think that the
same analysis holds here—these are exhaustive, and if a VASSAL class
exhibits any of these problems, then it should never have been released.

However, there is one exception to all of this that I can see rignt now:
If what’s thrown is an OutOfMemoryError (or has an OutOfMemoryError
as it’s ancestral cause) then it’s probably not a bug we’re seeing, and
so it should not be handled as one.

So, it looks like we want to do this in Buildable.create():

final String name = e.getTagName();
try {
b = (Buildable) (mod == null ?
Class.forName(name) : mod.getDataArchive().loadClass(name)).newInstance();
}
catch (Throwable t) {
// find and rethrow causes which are not bugs
for (Throwable c = t.getCause(); c != null; c = c.getCause()) {
if (c instanceof OutOfMemoryError) {
throw new OutOfMemoryError().initCause(t);
}
}

if (t instanceof ClassCastException ||
t instanceof ClassNotFoundException ||
t instanceof IllegalAccessException ||
t instanceof InstantiationException ||
t instanceof SecurityException ||
t instanceof ExceptionInInitializerError ||
t instanceof LinkageError e) {
// one of the standard classloading problems occured
if (name.startsWith(“VASSAL.”)) {
// this is a bug if the class is ours
ErrorDialog.bug(t);
}
else {
// otherwise this is probably a module bug
ErrorDialog.moduleBug(t);
}
}
else if (t instanceof Error) {
// some unusual problem occurred
throw (Error) t;
}
else if (t instanceof RuntimeException) {
// some unusual problem occurred
throw (RuntimeException) t;
}
else {
// this should never happen
throw new IllegalStateException();
}
}

Does this look reasonable?


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

The reason that Command and Builder trap all errors is just to guard against bad custom code in modules. Distinguishing between real bugs and module bugs is the right next step to take, so I like your logic for Builder. Although why not have a series of “catch(ClassCastException) … catch(ClassNotFoundException) …” instead of a single catch block and a bunch of “instanceof” checks?

In Command, we have less confidence that a runtime exception is necessarily a module bug, but we could use a simple heuristic. If the Command class is in a “VASSAL.*” package, report it as a bug, otherwise as a module bug.

rk

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Rodney Kinney”:

That’s what I’d guessed. Thanks for confirming it.

My reason for not having a series of catches is that no matter what
flavor of Throwable we’re catching, if there’s an OutOfMemoryError
anywhere in the transitive closure of getCause() then we should handle
it like an OutOfMemoryError rather than a VASSAL or module bug.

I know we could see an OutOfMemoryError wrapped inside an
ExceptionInInitializerError, which could happen if an OutOfMemoryError
was thrown in a static block or assignment to a static field, and we
could see an OutOfMemoryError wrapped in an InstantiationException
if an OutOfMemoryError was thrown in a Buildable’s ctor.

Those are just the two I know about, though. I have no idea if there
are more, but if we do it this way, we don’t have to care if there are
more, becuase they’ll be caught and handled. If we do it with a series
of catches, then we could miss some burried OutOfMemoryErrors and end
up treating them as bugs.

Why is that?


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

I just mean that an exception could be thrown by a custom class but caught in a Command class from the core engine (e.g. AddPiece), so even checking the package of the Command class could be misleading. Maybe you could examine the stack trace.

rk

Post generated using Mail2Forum (mail2forum.com)

Thus spake “Rodney Kinney”:

I think what’s needed is a way to distinguish among three kinds of classes:
ones which are part of VASSAL, ones which are JRE or library classses, and
ones which are part of a module. The reason you need three categories is
that seeing a JRE or library class in a stack trace isn’t enough to
distinguish between a VASSAL or a module problem. If you find an exception
was thrown in some java.foo.Bar, then you need to walk up the stack trace
until you find the first class which isn’t a system or library class, as
that class it the likely culprit. Once you’ve found that class, you have to
be able to tell whether it’s one of our classes.

It’s easy enough to find where a class came from:

Foo.class.getResource(“Foo.class”).toString()

will give you a URL pointing to the class file. For VASSAL classes, they
have to come from Vengine.jar, at least in a production copy. (This won’t
be the case when running a development copy via Eclipse, though.) We also
know where all of our library classes live, since we’re providing the JARs
for them. Many JRE classes live in rt.jar (though I’m unsure whether all
do). So long as these are exhaustive, then anything else belongs to the
module.

Alternately, we could assume that any class starting with ‘java.’,
‘javax.’, or ‘.sun’ is a JRE class, any class starting with ‘VASSAL.’ is
ours, we could check our libraries for their package names, and that
would leave whatever is left as a module class.

There might be some other way to tackle this as well.


J.


Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)