From my perspective of not having looked at the code for a couple
years these all seem like good changes.

thanks
david jencks

On Mar 16, 2007, at 2:13 PM, Andrew Goedhart wrote:

> I am working through the XA code specifically XAManagedConnection
> and XAConnectionFactory.
>
> I need to know how we go about submitting patches and also to ask
> questions in case I am missing something
>
> A.) Destroy()
>
> Firstly in destroy() of FBmanagedConnections
>
> public void destroy() throws ResourceException {
>
> if (gdsHelper == null)
> return;
>
> if (gdsHelper.inTransaction())
> throw new java.lang.IllegalStateException(
> "Can't destroy managed connection with active transaction");
>
> try {
> 1.) >>>> gdsHelper.getInternalAPIHandler().iscDetachDatabase
> (dbHandle);
> } catch (GDSException ge) {
> 2.) >>>> throw new FBResourceException("Can't detach from db.", ge);
> } finally {
> 3.) >>>> gdsHelper = null;
> }
> }
>
> Why are we throwing an exception if we have problems destroying the
> connection at 1.). By the time we return reach 2.) the physical
> connection is completely destroyed due to the finally in iscDetach
> () or due to a network error. This means the connection is no
> longer usable. It seems that in the JBoss connection manager this
> stops the connection from being properly destroyed and it gets
> recycled with devastating results. If we are interested in
> recognising the problem, can't we just log it instead. We even
> destroy out link to the underlying connection at 3. so we have no
> hope of reusing the connection.
>
> The GDSException can occur from a network error on the connection,
> so even if there is no problem with the way the connection used it
> can still stuff up JBOSS.
>
> Suggest changes:
> public void destroy() throws ResourceException {
>
> if (gdsHelper == null)
> return;
>
> if (gdsHelper.inTransaction())
> throw new java.lang.IllegalStateException(
> "Can't destroy managed connection with active transaction");
>
> try {
> gdsHelper.getInternalAPIHandler().iscDetachDatabase(dbHandle);
> } catch (GDSException ge) {
> if (log != null) log.trace(ge.getMessage());
> } finally {
> gdsHelper = null;
> }
> }
>
> Another issue the line 1.) which allows access to the underlying
> ApiHandler breaks encapsulation of the gdsHelper. This would
> normally be a style issue, but we have made network errors fatal.
> Typically helper functions in GDSHelper have the following format:
>
> public function (args)throws GDSException {
> try {
> do something ....
> } catch(GDSException ex) {
> notifyListeners(ex);
> throw ex;
> }
> }
> Now because we allow direct access to the GDS implementation we
> bypass this checking and those functions which are called directly
> and that result in a fatal GDSException do not cause the listeners
> to be notified.
>
> The simplest way of getting round this would be to make
> getInternalAPIHandler() return a proxy which wraps the underlying
> GDS implementation but checks for fatal GDS exceptions and notifies
> the listeners. I.e. change getInternalAPIHandler() to:
>
> /**
> * Get Firebird API handler (sockets/native/embeded/etc)
> *
> * @return handler object for internal API calls
> */
>
> public GDS getInternalAPIHandler() {
> return new GDSProxy(this.gds);
> }
>
> And now every attempt to call a function which can throw a
> GDSException is wrapped and delegated as follows:
>
> class GDSPRoxy {
> ...
> ...
> public void actionFn(args) throws GDSException {
> try {
> this.implmentation.actionFn(args)
> } catch(GDSException ex) {
> notifyListeners(ex);
> throw ex;
> }
> }
> ...
> ...
> }
>
> Complete code for changes to GdsHelper including proxy attached.
>
> 3.) Another problem I have being experiencing with the JBoss
> transaction manager and Jaybird is that when a attempt to start a
> transaction fails, it will always fail because the container will
> keep on calling start() before using the connection. Becasue we
> have not notifed the container the connection is broken it will
> keep on cycling it. It would be better to have connection
> discarded, because if we won't allow a connection to start
> something has gone badly wrong between Jaybird and the container.
> The chances are we will never get past this stage no matter how
> many times we try. We may as well notify the container the the
> connection is no longer usable. We should do this even if the
> GDSException is not one of the 3 fatal errors. The possible changes
> for start are given below:
>
> public void start(Xid id, int flags) throws XAException {
> try {
>
> // reset the transaction parameters for the managed scenario
> setTransactionIsolation(mcf.getDefaultTransactionIsolation());
>
> internalStart(id, flags);
>
> mcf.notifyStart(this, id);
>
> } catch (GDSException ge) {
> notifyFatalError(ge);
> throw new FBXAException(XAException.XAER_RMERR,ge);
> } catch(ResourceException ex) {
> notifyFatalError(ex);
> throw new FBXAException(XAException.XAER_RMERR, ex);
> }
> }
>
> /**
> * notify the container that the connection is no longer usabale.
> There are certain instances where
> * a connection cannot be recovered correctly after an error and
> therefore becomes unusable even
> * though it is not a generically fatal exception.
> * @param e the exception that caused this failure.
> */
> private void notifyFatalError(Exception e){
> ConnectionEvent event = new ConnectionEvent
> (FBManagedConnection.this,
> ConnectionEvent.CONNECTION_ERROR_OCCURRED, e);
> FBManagedConnection.this.notify(connectionErrorOccurredNotifier,
> event);
> }
>
> Comments ???
>
> I have compiled the changes and am testing them on the production
> servers( this is the only way I can get the load). If we don't get
> run away errors in 2 or 3 days I think it will have solved the
> problems with XA Transactions, Jaybird and JBoss that I have been
> experiencing. Please forgive the typos and bad English in the last
> few posts, its been to many nights with to little sleep, Another
> reason I would like( no need ) comments.
>
> Andrew
>