New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning log when a connection with transaction is going to be closed #11032

Merged
merged 5 commits into from Aug 27, 2017

Conversation

@motooka
Contributor

motooka commented Aug 12, 2017

If a developer writes a code like the one described below, it's so hard to debug. This warning log may help them.

$conn = ConnectionManager::get('my_connection');
try {
	$conn->begin();
	// some insert/update/delete operations
	if ($someCondition) {
		// bug : missing $conn->commit();
		return;
	}
	// some insert/update/delete operations
	$conn->commit();
}
catch(Exception $e) {
	$conn->rollback();
}

motooka added some commits Aug 12, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 12, 2017

Codecov Report

Merging #11032 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11032      +/-   ##
============================================
+ Coverage     94.35%   94.62%   +0.27%     
- Complexity    12309    12327      +18     
============================================
  Files           422      422              
  Lines         31137    31218      +81     
============================================
+ Hits          29378    29539     +161     
+ Misses         1759     1679      -80
Impacted Files Coverage Δ Complexity Δ
src/Database/Connection.php 93.46% <100%> (+0.13%) 87 <3> (+3) ⬆️
src/Cache/CacheEngine.php 89.36% <0%> (-4.26%) 19% <0%> (ø)
src/Cache/CacheRegistry.php 96% <0%> (-4%) 11% <0%> (ø)
src/Database/Statement/StatementDecorator.php 96.36% <0%> (-1.64%) 25% <0%> (+2%)
src/Database/Expression/TupleComparison.php 100% <0%> (ø) 26% <0%> (ø) ⬇️
src/Database/Statement/BufferedStatement.php 100% <0%> (ø) 15% <0%> (+2%) ⬆️
src/Database/QueryCompiler.php 99.09% <0%> (+0.92%) 46% <0%> (ø) ⬇️
src/Database/Schema/SqliteSchema.php 99.5% <0%> (+2.09%) 78% <0%> (+11%) ⬆️
src/Database/Expression/Comparison.php 100% <0%> (+5.61%) 35% <0%> (ø) ⬇️
src/Database/Driver/Sqlite.php 95.83% <0%> (+20.83%) 10% <0%> (ø) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dd21d8...fcf510e. Read the comment docs.

codecov-io commented Aug 12, 2017

Codecov Report

Merging #11032 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11032      +/-   ##
============================================
+ Coverage     94.35%   94.62%   +0.27%     
- Complexity    12309    12327      +18     
============================================
  Files           422      422              
  Lines         31137    31218      +81     
============================================
+ Hits          29378    29539     +161     
+ Misses         1759     1679      -80
Impacted Files Coverage Δ Complexity Δ
src/Database/Connection.php 93.46% <100%> (+0.13%) 87 <3> (+3) ⬆️
src/Cache/CacheEngine.php 89.36% <0%> (-4.26%) 19% <0%> (ø)
src/Cache/CacheRegistry.php 96% <0%> (-4%) 11% <0%> (ø)
src/Database/Statement/StatementDecorator.php 96.36% <0%> (-1.64%) 25% <0%> (+2%)
src/Database/Expression/TupleComparison.php 100% <0%> (ø) 26% <0%> (ø) ⬇️
src/Database/Statement/BufferedStatement.php 100% <0%> (ø) 15% <0%> (+2%) ⬆️
src/Database/QueryCompiler.php 99.09% <0%> (+0.92%) 46% <0%> (ø) ⬇️
src/Database/Schema/SqliteSchema.php 99.5% <0%> (+2.09%) 78% <0%> (+11%) ⬆️
src/Database/Expression/Comparison.php 100% <0%> (+5.61%) 35% <0%> (ø) ⬇️
src/Database/Driver/Sqlite.php 95.83% <0%> (+20.83%) 10% <0%> (ø) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dd21d8...fcf510e. Read the comment docs.

@markstory markstory added this to the 3.4.13 milestone Aug 12, 2017

@markstory markstory added the database label Aug 12, 2017

@markstory markstory self-assigned this Aug 13, 2017

Show outdated Hide outdated src/Database/Connection.php Outdated
@ravage84

This comment has been minimized.

Show comment
Hide comment
@ravage84

ravage84 Aug 15, 2017

Member

Could we prevent such bugs with a hint in the docs, may be?

Member

ravage84 commented Aug 15, 2017

Could we prevent such bugs with a hint in the docs, may be?

@markstory

Looks good to me. What do you think @lorenzo ?

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Aug 16, 2017

Member

Looks good to me as well, currently on mobile, but it seems like Travis did not pass for this change :(

Member

lorenzo commented Aug 16, 2017

Looks good to me as well, currently on mobile, but it seems like Travis did not pass for this change :(

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Aug 17, 2017

Member

There are now conflicts that need to be resolved before this can be merged.

Member

markstory commented Aug 17, 2017

There are now conflicts that need to be resolved before this can be merged.

@markstory markstory modified the milestones: 3.4.13, 3.5.0 Aug 17, 2017

Merge branch 'master' into warning-log-on-active-transaction
# Conflicts:
#	src/Database/Connection.php
{
if ($this->_transactionStarted && class_exists('Cake\Log\Log')) {
Log::warning('The connection is going to be closed but the transaction is still active.');
}

This comment has been minimized.

@markstory

markstory Aug 18, 2017

Member

Recently this destructor was removed. Wouldn't that obviate the need for this change?

@markstory

markstory Aug 18, 2017

Member

Recently this destructor was removed. Wouldn't that obviate the need for this change?

This comment has been minimized.

@lorenzo

lorenzo Aug 18, 2017

Member

No, it would still be the same

@lorenzo

lorenzo Aug 18, 2017

Member

No, it would still be the same

if ($this->_transactionStarted && class_exists('Cake\Log\Log')) {
Log::warning('The connection is going to be closed but the transaction is still active.');
}
unset($this->_driver);

This comment has been minimized.

@dereuromark

dereuromark Aug 18, 2017

Member

Should we keep this line out of it then?

@dereuromark

dereuromark Aug 18, 2017

Member

Should we keep this line out of it then?

This comment has been minimized.

@markstory

markstory Aug 18, 2017

Member

Yeah, we just removed that code.

@markstory

markstory Aug 18, 2017

Member

Yeah, we just removed that code.

This comment has been minimized.

@chinpei215

chinpei215 Aug 21, 2017

Member

I think it would be better to explain more detail about what has happened.
@motooka We have removed this unset() call in #11049. However, you have reverted the change in your merge commit accidentally. I cannot see any new changes in your merge commit. Could you please merge, not overwrite.
Also, I would suggest to use git rebase instead of git merge to solve conflicts. See this help for contributing code.

@chinpei215

chinpei215 Aug 21, 2017

Member

I think it would be better to explain more detail about what has happened.
@motooka We have removed this unset() call in #11049. However, you have reverted the change in your merge commit accidentally. I cannot see any new changes in your merge commit. Could you please merge, not overwrite.
Also, I would suggest to use git rebase instead of git merge to solve conflicts. See this help for contributing code.

@markstory markstory modified the milestones: 3.5.0, 3.5.1 Aug 19, 2017

@markstory markstory merged commit fcf510e into cakephp:master Aug 27, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/patch 100% of diff hit (target 94.35%)
Details
codecov/project 94.62% (+0.27%) compared to 4dd21d8
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
stickler-ci No lint errors found.

markstory added a commit that referenced this pull request Aug 27, 2017

Emit log messages when connections are destroyed mid transaction
Merge branch 'issue-11032' into master.

Refs #11032
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment