[JBoss JIRA] (JBTM-1431) LastResourceRecord.shouldAdd allows insertion regardless of order and type info
by Mark Little (JIRA)
[ https://issues.jboss.org/browse/JBTM-1431?page=com.atlassian.jira.plugin.... ]
Mark Little closed JBTM-1431.
-----------------------------
Resolution: Done
> LastResourceRecord.shouldAdd allows insertion regardless of order and type info
> -------------------------------------------------------------------------------
>
> Key: JBTM-1431
> URL: https://issues.jboss.org/browse/JBTM-1431
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Security Level: Public(Everyone can see)
> Components: Transaction Core
> Affects Versions: 4.17.3
> Reporter: Mark Little
> Assignee: Mark Little
>
> So looking at the code, LastResource is meant to be ordered last in the intentions list (check topLevelPrepare to see this). However, a quick look at the code in LastResourceRecord shows that there is a bug:
> public boolean shouldAdd (AbstractRecord a)
> {
> if (a.typeIs() == typeIs())
> {
> if (ALLOW_MULTIPLE_LAST_RESOURCES) {
> if (!_disableMLRWarning
> || (_disableMLRWarning && !_issuedWarning)) {
> tsLogger.i18NLogger.warn_lastResource_multipleWarning(a.toString());
> _issuedWarning = true;
> }
> return true;
> }
> else {
> tsLogger.i18NLogger.warn_lastResource_disallow(this.toString(), a.toString());
> return false;
> }
> }
> else
> {
> return true; <------- Here is the bug!
> }
> }
> Basically if the record to be added is not a LastResourceRecord then it gets added immediately, ignoring any further checks on type, order etc. This is wrong.
> And looking back at the code from 6 years ago we see that this was much simpler:
> public boolean shouldAdd (AbstractRecord a)
> {
> return (a.typeIs() == typeIs()) ;
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1430) Restore commit method to AtomicAction
by Tom Jenkinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-1430?page=com.atlassian.jira.plugin.... ]
Tom Jenkinson commented on JBTM-1430:
-------------------------------------
OK, the public API is sorted: https://github.com/jbosstm/narayana/commit/43e7bdd94cce6ec051587dcdc43313...
There are number of internal package classes that have added methods purely for tests, I have looked at each one to see if I can change it to use a standard API easily. There are a significant number where this is feasible, however some cases would require moving the classes to other packages and changes to the state variables to allow them to be picked up as package scoped state. tbh I would prefer to just leave the internal code as deprecated so devs avoid using them in future and so we don't build up a test suite dependent on shortcuts into the code.
> Restore commit method to AtomicAction
> -------------------------------------
>
> Key: JBTM-1430
> URL: https://issues.jboss.org/browse/JBTM-1430
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Security Level: Public(Everyone can see)
> Components: Transaction Core
> Affects Versions: 4.17.3
> Reporter: Mark Little
> Assignee: Tom Jenkinson
>
> At some point someone deprecated the commit method in AtomicAction and left the following comment:
> @deprecated Only used by tests
> This is wrong. Just because we don't use the commit method in the code does not mean it is invalid in the scope of the public API. The commit method without the boolean parameter is a helper method to simplify calling the other commit with true all of the time. This was added years ago based on feedback from the OTS Current implementation, which had (in C++) a default boolean=true parameter in its signature.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1431) LastResourceRecord.shouldAdd allows insertion regardless of order and type info
by Mark Little (JIRA)
Mark Little created JBTM-1431:
---------------------------------
Summary: LastResourceRecord.shouldAdd allows insertion regardless of order and type info
Key: JBTM-1431
URL: https://issues.jboss.org/browse/JBTM-1431
Project: JBoss Transaction Manager
Issue Type: Bug
Security Level: Public (Everyone can see)
Components: Transaction Core
Affects Versions: 4.17.3
Reporter: Mark Little
Assignee: Mark Little
So looking at the code, LastResource is meant to be ordered last in the intentions list (check topLevelPrepare to see this). However, a quick look at the code in LastResourceRecord shows that there is a bug:
public boolean shouldAdd (AbstractRecord a)
{
if (a.typeIs() == typeIs())
{
if (ALLOW_MULTIPLE_LAST_RESOURCES) {
if (!_disableMLRWarning
|| (_disableMLRWarning && !_issuedWarning)) {
tsLogger.i18NLogger.warn_lastResource_multipleWarning(a.toString());
_issuedWarning = true;
}
return true;
}
else {
tsLogger.i18NLogger.warn_lastResource_disallow(this.toString(), a.toString());
return false;
}
}
else
{
return true; <------- Here is the bug!
}
}
Basically if the record to be added is not a LastResourceRecord then it gets added immediately, ignoring any further checks on type, order etc. This is wrong.
And looking back at the code from 6 years ago we see that this was much simpler:
public boolean shouldAdd (AbstractRecord a)
{
return (a.typeIs() == typeIs()) ;
}
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1398) Review subsystem usage across Narayana
by Paul Robinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-1398?page=com.atlassian.jira.plugin.... ]
Paul Robinson commented on JBTM-1398:
-------------------------------------
More details about the cyclic dep:
{code}
The projects in the reactor contain a cyclic reference: Edge between 'Vertex{label='org.jboss.as:jboss-as-weld:7.2.0.Alpha1-SNAPSHOT'}' and 'Vertex{label='org.jboss.as:jboss-as-ejb3:7.2.0.Alpha1-SNAPSHOT'}' introduces to cycle in the graph org.jboss.as:jboss-as-ejb3:7.2.0.Alpha1-SNAPSHOT --> org.jboss.as:jboss-as-connector:7.2.0.Alpha1-SNAPSHOT --> org.jboss.as:jboss-as-transactions:7.2.0.Alpha1-SNAPSHOT --> org.jboss.as:jboss-as-weld:7.2.0.Alpha1-SNAPSHOT --> org.jboss.as:jboss-as-ejb3:7.2.0.Alpha1-SNAPSHOT -> [Help 1]
{code}
Notice that each dependency is one on a jboss-as module, rather than an external dep.
> Review subsystem usage across Narayana
> --------------------------------------
>
> Key: JBTM-1398
> URL: https://issues.jboss.org/browse/JBTM-1398
> Project: JBoss Transaction Manager
> Issue Type: Task
> Security Level: Public(Everyone can see)
> Components: TXFramework
> Reporter: Paul Robinson
> Assignee: Paul Robinson
> Fix For: 5.0.0.M2
>
> Original Estimate: 2 days
> Time Spent: 1 day, 5 hours
> Remaining Estimate: 3 days
>
> h2. Components and their subsystem requirements:
> h5. Transactions
> * To Investigate
>
> h5. XTS
> * Bootstrap coordinator
> * Setup WS endpoints for coordinator
> * Setup WS endpoints for the participants
> * Respond to configuration from standalone-xts.xml
> h5. REST-TX
> * Boostrap the coordinator
> * Setup a REST endpoint for the coordinator
> * Setup a REST endpoint for the participants
> h5. Blacktie
> * Register MDB
> * Register MBean
> h5. STM
> * To Investigate
> h5. TXF
> * Modify the WS handler chain on application deployment
> * Registers a CDI extension
> h2. Notable Dependencies
> h5. TXF
> * Weld -> EJB3 -> Transactions (Could check that each link is valid. Check if subsys or regular dep)
> * JBossWS. Might be able to remove by putting the WS specific stuff into the XTS subsystem.
> h2. Subsystem Breakdown
> Providing we can have 'soft' dependencies it would seem favourable to have all the core features in the transactions subsystem and the optional 'transports' in additional subsystems.
> h5. Transactions
> * All current stuff
> * STM
> * TXF (Unlikely to work due to dependency on Weld for loading a portable extension)
> h5. XTS
> * All Current Stuff
> h5. RTS (new)
> h5. Blacktie (new)
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1430) Restore commit method to AtomicAction
by Tom Jenkinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-1430?page=com.atlassian.jira.plugin.... ]
Tom Jenkinson commented on JBTM-1430:
-------------------------------------
Hi Mark,
I will take a look and make sure that the "public" methods are no longer deprecated as you say, we can't really anticipate what users are utilizing and there could be good reason for them to exist - I don't know why I didn't filter that before.
-- I will make this change immediately.
I would argue that maintaining code in the internal packages that are purely used by tests is not ideal as it affects coverage and further gives us more code to maintain, hence I deprecated that code to ensure new tests were not written using these helper methods when similar APIs that are utilized heavily were provided.
-- I will review these individually in the context of the test.
Tom
> Restore commit method to AtomicAction
> -------------------------------------
>
> Key: JBTM-1430
> URL: https://issues.jboss.org/browse/JBTM-1430
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Security Level: Public(Everyone can see)
> Components: Transaction Core
> Affects Versions: 4.17.3
> Reporter: Mark Little
> Assignee: Tom Jenkinson
>
> At some point someone deprecated the commit method in AtomicAction and left the following comment:
> @deprecated Only used by tests
> This is wrong. Just because we don't use the commit method in the code does not mean it is invalid in the scope of the public API. The commit method without the boolean parameter is a helper method to simplify calling the other commit with true all of the time. This was added years ago based on feedback from the OTS Current implementation, which had (in C++) a default boolean=true parameter in its signature.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1430) Restore commit method to AtomicAction
by Mark Little (JIRA)
[ https://issues.jboss.org/browse/JBTM-1430?page=com.atlassian.jira.plugin.... ]
Mark Little commented on JBTM-1430:
-----------------------------------
It turns out that several methods have been deprecated with the same "reason" comment. These should be reviewed, because the reason is spurious at best.
> Restore commit method to AtomicAction
> -------------------------------------
>
> Key: JBTM-1430
> URL: https://issues.jboss.org/browse/JBTM-1430
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Security Level: Public(Everyone can see)
> Components: Transaction Core
> Affects Versions: 4.17.3
> Reporter: Mark Little
> Assignee: Tom Jenkinson
>
> At some point someone deprecated the commit method in AtomicAction and left the following comment:
> @deprecated Only used by tests
> This is wrong. Just because we don't use the commit method in the code does not mean it is invalid in the scope of the public API. The commit method without the boolean parameter is a helper method to simplify calling the other commit with true all of the time. This was added years ago based on feedback from the OTS Current implementation, which had (in C++) a default boolean=true parameter in its signature.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1430) Restore commit method to AtomicAction
by Mark Little (JIRA)
Mark Little created JBTM-1430:
---------------------------------
Summary: Restore commit method to AtomicAction
Key: JBTM-1430
URL: https://issues.jboss.org/browse/JBTM-1430
Project: JBoss Transaction Manager
Issue Type: Bug
Security Level: Public (Everyone can see)
Components: Transaction Core
Affects Versions: 4.17.3
Reporter: Mark Little
Assignee: Tom Jenkinson
At some point someone deprecated the commit method in AtomicAction and left the following comment:
@deprecated Only used by tests
This is wrong. Just because we don't use the commit method in the code does not mean it is invalid in the scope of the public API. The commit method without the boolean parameter is a helper method to simplify calling the other commit with true all of the time. This was added years ago based on feedback from the OTS Current implementation, which had (in C++) a default boolean=true parameter in its signature.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1311) Unify all XTS/localjunit tests
by Paul Robinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-1311?page=com.atlassian.jira.plugin.... ]
Paul Robinson logged work on JBTM-1311:
---------------------------------------
Author: Paul Robinson
Created on: 16/Jan/13 6:01 AM
Start Date: 16/Jan/13 6:01 AM
Worklog Time Spent: 5 minutes
Issue Time Tracking
-------------------
Remaining Estimate: 0 minutes (was: 4 hours)
Time Spent: 4 hours, 5 minutes (was: 4 hours)
Worklog Id: (was: 12428407)
> Unify all XTS/localjunit tests
> -------------------------------
>
> Key: JBTM-1311
> URL: https://issues.jboss.org/browse/JBTM-1311
> Project: JBoss Transaction Manager
> Issue Type: Task
> Security Level: Public(Everyone can see)
> Components: Testing, XTS
> Reporter: Paul Robinson
> Assignee: Paul Robinson
> Priority: Minor
> Fix For: 4.17.4, 5.0.0.M2
>
> Original Estimate: 1 day
> Time Spent: 4 hours, 5 minutes
> Remaining Estimate: 0 minutes
>
> All the XTS/localjunit tests run with Arquillian and should have a consistent configuration. Therefore we should be able to put all the config in the XTS/localjunit/pom.xml. However, the crash recovery tests in particular have a there own properties and Arquillian config.
> Also, the test don't have a consistent arquillian.xml. We should try to make this consistent too
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months
[JBoss JIRA] (JBTM-1311) Unify all XTS/localjunit tests
by Paul Robinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-1311?page=com.atlassian.jira.plugin.... ]
Paul Robinson updated JBTM-1311:
--------------------------------
Status: Resolved (was: Pull Request Sent)
Resolution: Done
> Unify all XTS/localjunit tests
> -------------------------------
>
> Key: JBTM-1311
> URL: https://issues.jboss.org/browse/JBTM-1311
> Project: JBoss Transaction Manager
> Issue Type: Task
> Security Level: Public(Everyone can see)
> Components: Testing, XTS
> Reporter: Paul Robinson
> Assignee: Paul Robinson
> Priority: Minor
> Fix For: 4.17.4, 5.0.0.M2
>
> Original Estimate: 1 day
> Time Spent: 4 hours, 5 minutes
> Remaining Estimate: 0 minutes
>
> All the XTS/localjunit tests run with Arquillian and should have a consistent configuration. Therefore we should be able to put all the config in the XTS/localjunit/pom.xml. However, the crash recovery tests in particular have a there own properties and Arquillian config.
> Also, the test don't have a consistent arquillian.xml. We should try to make this consistent too
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
13 years, 2 months