Fixed: XAER_RMFAIL cascade causing connection pool exhaustion during XA transactions - #733
Closed
dixitdeepak wants to merge 143 commits into
Closed
Fixed: XAER_RMFAIL cascade causing connection pool exhaustion during XA transactions#733dixitdeepak wants to merge 143 commits into
dixitdeepak wants to merge 143 commits into
Conversation
Merge upstream
…e differently for EntityListIterator to avoid calling hasNext(); this improves performance and helps with a MySQL JDBC driver issue with useCursorFetch = true where ResultSet.isBeforeFirst() returns true when there are no results, and it should return false in that case (cherry picked from commit e6a7b8e)
…Actions iterator element did not auto close EntityListIterator instances; thanks to Deepak for reporting this on the forum (cherry picked from commit d640e8d)
…nclude the catalog from the Connection; this is needed for MySQL which does not restrict meta data queries to the current connected database without specifying a catalog on these methods; this is tested with H2 and should be fine with Postgres as well, may cause issues with other databases and needs more testing
In EntityDbMeta change all calls to getTables() and getColumns() to i…
Upstream sync
…lTemplateRenderer and MNode class (moqui#565) https://freemarker.apache.org/docs/versions_2_3_32.html
…d testing of service calls with some profiling info, some basics for a first pass
…ail, part of code is more generic addition to ArtifactExecutionInfoImpl
… in addition to entire run stats, prep work for performance charts and such
* Fixed message queue clearance logic in ec.message.clearAll() method * switch to gradle 8.14.1 * address gradle 8 warnings * more gradle 8.14 fixes * fixed to gradle 8.14 * equal assignment to descriptions, upgrade gradle 8.14 * bump gradle to 8.14.3 * apply fixes to gradle in moqui this resolves all warnings except java version * bump to gradle 9.2 * ignore configuration cache for now * switch to newest bitronix with jakarta JTA * fix stopSearch to work with gradle 9+ * replace the rest of the exec commands * default to opensearch and fix issues * default to JDK 21 * upgrade first version of release notes * already chucked out javassist from bitronix * Update commons-lang3 and commons-beanutils versions * allow unit tests to run under gradle 9 * add convenience tasks for testing everything * fix failing cache facade test * Fixed the issue that OpenSearch failed to start on macOS. (moqui#661) * remove redundant finalize methods, destroy exists This helps removes warnings about non deterministic finalize which is deprecated in newer java versions * remove finalize, nothing to cleanup just a boolean * remove finalize, already cleaned up correctly close() is being called when getCompleteList(..., closeAfter) and getPartialList(..., closeAfter). So redundant code and non deterministic and deprecated anyway, might never be called by the JVM * remove deprecated finalize resource is already autoclosable by contract * centralize dependencyUpdates and gradle 9 formatting * upgrade all libraries, shiro now version 2 Big change, shiro bumped to version 2, one change in behavior is that logins do not survive restarts. So I added the remember functionality in a secure way. By default production instances will not survive login restarts UNLESS rememberme key is generated and passed to the instance * remove incorrect implementation of remember me * remove shiro.ini from tests, it's no longer needed * upgrade libraries and handle changes to API * update changelog to incorporate all the work * go back to INI * fix typo now that we switched to env INI * breakout statements to make @CompileStatic happy * no fallback to empty string again * return shiro.ini to tests * fix remaining shiro2 bugs * upgrade to groovy 5 * first round of jakarta / jetty upgrades * switch from javax.xml.bind to jakarta * reset to pre-groovy and expand imports * Solve most issues in MoquiStart.java Only one bug remains: Failed startup of context oeje10w.WebAppContext@2f9a01c1{ROOT,/,b=file:///home/taher/Desktop/active/repositories/fork/moqui-framework/execwartmp/ROOT/webapp/,a=STOPPED,h=oeje10s.SessionHandler@2611b9a3{STOPPED}}{file:/home/taher/Desktop/active/repositories/fork/moqui-framework/moqui.war} java.lang.IllegalArgumentException: URI is not hierarchical So we need to find a solution for web.xml being inside the war file * transitional commit, still WIP for jetty / jakarta * cleanup jetty implementation in MoquiStart, still WIP * update and cleanup comments * jetty logs down to warning from error * remove another asterisk import * go back to persisted sessions * update comments to reflect persisted sessions * add missing comment on license * remove useless comments * code self explanatory * add comments on groovy 5 * Run Groovy shell evaluations on a dedicated executor thread Execute Groovy shell evaluations off the WebSocket thread to avoid blocking I/O. Introduce a volatile closing flag to coordinate async shutdown and prevent races. Add idle and evaluation timeouts to protect against leaked sessions and long-running or stuck scripts. Ensure all async paths are guarded with try/catch and consistently clean up ExecutionContext, timers, and executor resources on close. * absolutely no need to check against null * restore signature of flush and close * move from ee10 to ee11 also reintroduce servlet listener that was removed with proper updated version * Fix a bug introduced by Jetty confusing APIs In MoquiStart.java, I have this very specific very clear instruction: webappClass.getMethod("setClassLoader", ClassLoader.class).invoke(webapp, moquiStartLoader); Yet, in MClassLoader, I added these three lines to debug: System.out.println("=== MClassLoader constructor ==="); System.out.println("MCL = " + this); System.out.println("MCL parent = " + parent); AND, look at the outcome: === MClassLoader constructor === MCL = org.moqui.util.MClassLoader@77f991c MCL parent = oejew.WebAppClassLoader{Moqui Root Webapp}@6bea52d4 Starting MClassLoader with parent org.eclipse.jetty.ee.webapp.WebAppClassLoader Jetty is injecting its WebAppClassLoader even though there is a clear instruction for it to disregard that class loader. Their API is confusing at best! This solution solved classpath problems properly by making MClassLoader a child of StartClassLoader as intended. * update ReleaseNotes to reflect all the updates * upgrade all infra to latest version Pending thorough testing. Initial tests seem to be OK * upgrade release notes to include infra * better organize and cleanup notes * Fix the class loading issue from MClassLoader and preserve everything * reintroduce char * slightly improve MoquiStart comments * find closest compatible call without altering structure * less noise from jetty annotations * limit to 2 parents for efficiency * restore original comment with "parent chain" fix * cleanup and simplify * upgrade headers for jakarta * provide better documentation for how class loading works * update comment to match updated call * add useful comment on why join() * Finally solve last classpath issue With this fix, moqui runs properly on both embedded mode AND when deployed on a servlet container. Need to add documentation on how to test in a servlet container. * move outside execWar, candidates eventually to delete * logging not used in MoquiStart * cleanup unused dependencies * fix a bug in jetty 12.1 EE11 on websocket sessions Apparently in jetty 12 on EE10+ due to the separation of websockets and EE stack from server stack, we cannot retrieve the HTTP session from websocket handshake. Therefore fixing the bug in both UserFacade and MoquiAbstractEndpoint accordingly. jetty/jetty.project#11809 * fix the same bug with respect to fetching ip address * Revert "fix the same bug with respect to fetching ip address" This reverts commit 2afc70c. * cleanup dependencies & apply futher jakarta names * improve the comment for maximum clarity * correct for running tests properly * fix remaining jetty web socket based session bugs * fully tested and solved sending emails * add note on jakarta switch * comment too old, no longer applicable * update comment on heap size * move to first stable version of bitronix * fix the expose incorrect keyword * fix issues with upgrading infrastructure: - opensearch generates more files, added them to gitignore - moqui creates two unnamed volumes because of dockerfile. The right solution is to add them in compose files as runtime binds - US/Pacific is no longer an acceptable timezone for postgres so using standard UTC for safety on all compose files - opensearch enforces very strictly a password with minimum requirements that cannot be overridden. Therefore changing it in all compose files * Add a basic README.md in docker to explain how to use it * add more thorough documentation to README * a few more improvements to README in docker * remove "expose" key, not allowed in recent docker-compose * remove generated conf files * bump Configuration.VERSION of freemarker to match * fix the framework version to 4.0.0 * Allow running gradle getRuntime * Create setupVscode gradle task * Make postgres_backup keep have better default for what data is saved * add note on how to override default image * Fix ./gradlew setupVsCode * fix: exclude old jakarta.mail and jakarta.activation dependencies from 'org.apache.commons:commons-email2-jakarta:2.0.0-M1 * remove outdated comment * another outdated comment * revert earlier work causing elastic search bugs * these tasks are not needed, test runs for all components It turns out the issue was not running all tests, but rather one failure stops all components * change EntityValue API to solve ambiguity bug Groovy 4+ introduced a change in the way property to method mapping happens as [documented](https://groovy-lang.org/style-guide.html#_getters_and_setters). This introduced a bug that occurs when querying an entity that has a field named `entityName`. The bug occurs because the query returns an object of type `org.moqui.entity.EntityValue`. The problem is that the EntityValue class has a method called getEntityName() and as per the groovy 4+ behavior this function is called when trying to access a field named `entityName`. Sample code: ``` def someMember = ec.entity .find('moqui.entity.view.DbViewEntityMember') .condition(...) .one() someMember.entityName // BUG returns .getEntityName(), not .get('entityName') ``` * fix bug in wiki services missing function call * Fix RestClient idleTimeout ignoring timeoutSeconds > 30 * change TZ to UTC to work with docker databases * Update addons.xml * fix bug where image cannot receive commands (load, help) * forgot to update the yaml files * Update addons with pre moqui 4.0 release version tags, and fix for no versions * Update all addons.xml components with versions for latest code * Update versions and documentation for current release * Update addons.xml versions and fix RestClient CompletableResponseListener * Update addons.xml versions and fix RestClient CompletableResponseListener * Update badge links for version 4.0.0 * Add get jdbc for postgres and mysql gradle task, add moquidemo actions release * Update workflow to include additional components * Add mavenCentral and remove moquidemo release * switch to generated bitronix jar --------- Co-authored-by: Taher Alkhateeb <taher@pythys.com> Co-authored-by: Acetousk <acetousk@users.noreply.github.com> Co-authored-by: Wei Zhang <zhangwei@apache.org> Co-authored-by: Acetousk <29029373+acetousk@users.noreply.github.com> Co-authored-by: Jens Hardings <jhp@moit.cl> Co-authored-by: Karthik Ks <karthik.ks@azentio.com>
…to sync-release-4.0
Sync release 4.0
Local development clones full history with all branches by default. Pass -PshallowClone=true during CI/deployment to use --depth 1, reducing clone time and repository size.
In makeAhiValue, parameters may contain ContextStack values (which implement Map). The previous check order called ObjectUtilities.isEmpty(value) before value instanceof Map, causing ContextStack.size() to traverse the stack and hit a recycled Jetty EE11 request object in a background thread — resulting in a NullPointerException from ServletApiRequest.getRequest() returning null. Fix by checking instanceof Map/Collection before isEmpty so ContextStack values short-circuit without calling size(). Also add a defensive NullPointerException catch in ServletRequestContainer.getAttributeNames() to mirror the existing IllegalStateException guard in HttpSessionContainer.
Removed early closeTxConnections() calls in commit() and rollback() methods of TransactionFacadeImpl. This prevents connections from being returned to the pool before the JTA transaction ends, which was causing XAER_RMFAIL errors on other threads.
Fix XA connection pool pollution on commit and rollback
Improved: Added support to clone release candidate tags.
…XA transactions
Root cause:
When MySQL marks an XA branch as rollback-only (e.g. after a deadlock or timeout),
Bitronix auto-enlistment fails with XAER_RMFAIL on the next statement call. Because
PoolingDataSource does not implement javax.sql.XADataSource, Moqui's getConnection()
bypasses enlistConnection() entirely and calls ds.getConnection() directly, returning a
ConnectionJavaProxy. When XA START is rejected (XAER_RMFAIL), the connection is never
enrolled in the TX, but Bitronix's release() still requeues it to the pool with the dirty
XA branch intact. Every subsequent checkout of that connection fails the same way,
producing a cascade (2000+ XAER_RMFAIL errors within seconds).
Fix — ContextJavaUtil.java (ConnectionWrapper):
- Added volatile destroyOnClose flag.
- Added isXaRmFail() that traverses the exception cause chain checking for
XAException.XAER_RMFAIL (JTA spec constant -7). Avoids fragile string matching on
exception messages which are locale/driver-version sensitive.
- Wrapped createStatement(), prepareStatement(String), and prepareCall(String) with
try-catch: sets destroyOnClose = true on XAER_RMFAIL and rethrows. Only the three base
variants are wrapped because Bitronix auto-enlistment fires once on the first statement
call, and EntityQueryBuilder exclusively uses prepareStatement(String sql).
- closeInternal() calls physicallyDestroy() when destroyOnClose is true.
physicallyDestroy() uses reflection to invoke JdbcPooledConnection.close() via
ConnectionJavaProxy.getPooledConnection(), which unregisters the connection from the
Bitronix pool and closes the physical DB connection, preventing the dirty XA branch
from being recycled. Falls back to con.close() if reflection fails (non-Bitronix pools).
Fix — TransactionFacadeImpl.groovy:
- suspend(): removed closeTxConnections() before tm.suspend(). Stashed connections belong
to the outer TX's suspended XA branches and must not be returned to pool mid-suspend;
they are released when the outer TX resumes and eventually commits or rolls back.
- enlistConnection(): added finally { if (con.close() } to destroy the
XAConnection on enlistment or connection-retrieval failure for raw XADataSource paths
(non-Bitronix), preventing the same dirty-connection cascade on those configurations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause:
When MySQL marks an XA branch as rollback-only (e.g. after a deadlock or timeout),
Bitronix auto-enlistment fails with XAER_RMFAIL on the next statement call. Because
PoolingDataSource does not implement javax.sql.XADataSource, Moqui's getConnection()
bypasses enlistConnection() entirely and calls ds.getConnection() directly, returning a
ConnectionJavaProxy. When XA START is rejected (XAER_RMFAIL), the connection is never
enrolled in the TX, but Bitronix's release() still requeues it to the pool with the dirty
XA branch intact. Every subsequent checkout of that connection fails the same way,
producing a cascade (2000+ XAER_RMFAIL errors within seconds).
Fix — ContextJavaUtil.java (ConnectionWrapper):
XAException.XAER_RMFAIL (JTA spec constant -7). Avoids fragile string matching on
exception messages which are locale/driver-version sensitive.
try-catch: sets destroyOnClose = true on XAER_RMFAIL and rethrows. Only the three base
variants are wrapped because Bitronix auto-enlistment fires once on the first statement
call, and EntityQueryBuilder exclusively uses prepareStatement(String sql).
physicallyDestroy() uses reflection to invoke JdbcPooledConnection.close() via
ConnectionJavaProxy.getPooledConnection(), which unregisters the connection from the
Bitronix pool and closes the physical DB connection, preventing the dirty XA branch
from being recycled. Falls back to con.close() if reflection fails (non-Bitronix pools).
Fix — TransactionFacadeImpl.groovy:
to the outer TX's suspended XA branches and must not be returned to pool mid-suspend;
they are released when the outer TX resumes and eventually commits or rolls back.
XAConnection on enlistment or connection-retrieval failure for raw XADataSource paths
(non-Bitronix), preventing the same dirty-connection cascade on those configurations.