spacewalkproject/spacewalk — pull request #191 by moio (2014-12-04 10:24:38+00:00)
Title: Don't commit when XMLRPCExceptions are thrown

Avoid `COMMIT`ting the current Hibernate transaction if an uncaught Exception is thrown.

Reproducible by adding the following snippet to `SystemHandler`:

public void testExceptionHandling() {
TinyUrl tu = CommonFactory.createTinyUrl(TestUtils.randomString(), new Date());
CommonFactory.saveTinyUrl(tu);
throw new XmlRpcException("boom");
}

And then calling it, for example, via:

#!/usr/bin/env ruby
require "xmlrpc/client"

client = XMLRPC::Client.new2("http://s21pg.moio/rpc/api")
client.call('system.testExceptionHandling')

A new row will be saved in `rhnTinyUrl` (!!!)

### Why does this happen

As all other servlet requests, XMLRPC calls route through `SessionFilter`, which handles opening, committing, and rolling back the Hibernate transaction (in case there are uncaught exceptions). Relevant snippet:

try {
// ...
chain.doFilter(request, response); // CUSTOM CODE ENTRY POINT
HibernateFactory.commitTransaction();
logHere("Transaction committed");
committed = true;
}
// ...
finally {
try {
if (!committed) {
try {
logHere("Rolling back transaction");
HibernateFactory.rollbackTransaction();

Problem is that `doFilter`, in case of XMLRPC APIs, indirectly calls `reedstone.xmlrpc.XmlRpcDispatcher.dispatch(InputStream, Writer)`, a method internal of the [Redstone XMLRPC library](http://xmlrpc.sourceforge.net/) that swallows all exceptions!

// ...
try {
// ...
Object returnValue = handler.invoke( methodName, arguments ); // CUSTOM CODE ENTRY POINT
}
catch ( Throwable t )
{
processException( invocation, t );
int code = -1;
if ( t instanceof XmlRpcFault )
{
code = ( (XmlRpcFault) t ).getErrorCode();
}
writeError( code, t.getClass().getName() + ": " + t.getMessage() );
}

This means that no exception gets to `SessionFilter`, which will happily always commit.

### Why does the patch work

`BaseHandler` is always called by `XmlRpcDispatcher` to get to the custom classes, in case an exception is caught, it will roll back and rethrow it. `SessionFilter` will continue to commit always, so this is kind of a hack.

OTOH there's no easy way to get to `SessionFilter` as exceptions are swallowed by `XmlRpcDispatcher`, unless we want to patch Redstone itself.