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

commit/rollback multiple times when not using CLS #4491

Closed
arcana261 opened this Issue Sep 14, 2015 · 2 comments

Comments

3 participants
@arcana261
Contributor

arcana261 commented Sep 14, 2015

Hi.

I had an issue as of late using latest stable version of Sequelize on our proprietary app. We don't use CLS (manual transaction life-cycle management on our side) and are on PostgreSQL. I would receive multiple instances of SET TRANSACTION ISOLATION LEVEL must be called before any query errors. This problem was devastating because connections in connection pool were exhausted and as a result all REST APIs to our server were timed-out. I narrowed the problem down to executing rollback on the same transaction multiple times.

So I think that there are two problems with Sequelize. First is that it allows rollback/commit on a transaction multiple times and thus it releases (pools) the same connection multiple times. Second is that if any error should happen while initializing a new transaction (promise chain in method prepareEnvironment) a connection is lost and exhausted. I can not rollback such a transaction on my side because transaction context object is not given to me in the first place, if such error is to occur.

I understood that by doing so, the connection is pooled multiple times and thus can be acquired and used in two different transactions concurrently. I'm fixing this in my code but nevertheless sequelize should not have had pooled the same connection multiple times.

In order to fix the first problem, I suggest adding a guard if to lib/transaction.js method rollback and commit so that any transaction is not rolleback/committed multiple times.

In order to fix the second problem, I suggest that in method prepareEnvironment we should check for any error during transaction startup, if any error should happen we should rollback half-initialized transaction (to fix connection exhaust bug).

I will make a pull request soon.

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 14, 2015

A pull request sounds good, it's hard to reason about code changes when they're described in plain text ;-)

@arcana261

This comment has been minimized.

Contributor

arcana261 commented Sep 14, 2015

made a pull request by the same name. I cant run the tests right now but I think they would be ok, Not so many lines are changed and I've done no magick 😄

@mickhansen mickhansen closed this Sep 14, 2015

Americas added a commit to Americas/sequelize that referenced this issue Sep 14, 2015

Added check to prevent multiple commit/rollback on a single transaction
when not using CLS sequelize#4491.
Added rollback to prepareEnvironment to abort half-initialized
transactions that run into error, fixes connection pool exhaust bug sequelize#4491.

Americas added a commit to Americas/sequelize that referenced this issue Sep 14, 2015

Added check to prevent multiple commit/rollback on a single transaction
when not using CLS sequelize#4491.
Added rollback to prepareEnvironment to abort half-initialized
transactions that run into error, fixes connection pool exhaust bug sequelize#4491.

Americas added a commit to Americas/sequelize that referenced this issue Sep 14, 2015

Added check to prevent multiple commit/rollback on a single transaction
when not using CLS sequelize#4491.
Added rollback to prepareEnvironment to abort half-initialized
transactions that run into error, fixes connection pool exhaust bug sequelize#4491.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment