Fix race condition in non-container transactions

[briandamaged]

Minor cleanup to some of the Transaction logic. However, it looks like this cleanup might have also inadvertently fixed the race condition that was being investigated here:

#3665

cc: @maximelkin + @kibertoad

[briandamaged]

@maximelkin : I'm fairly certain that this change is the one that fixed the race condition.

Your PR (#3665) included this change as well; however, it relocated the .catch((err)=> {throw err}) logic to a different part of the code. In contrast, this PR just removes that logic entirely.

[maximelkin]

This catch added in #3328, so I am unsure about this changes

[briandamaged]

This was just a minor code cleanup. It turns out that this._previousSibling is always defined due to the way the constructor is written. So, there was no need for conditional logic here.

Additionally, take note of the TODO. I'm 95% sure that the code should not be ignoring that exception. However, I left it in place for now so that we can minimize the impact of this PR.

[briandamaged]

Actually... I suspect that the .catch(()=> {}) line was the reason for the "silent failures" mentioned here:

#3328 (comment)

I'm going to test out a different implementation that will (hopefully!) allow for proper sibling->sibling linking without swallowing exceptions.

[briandamaged]

Alright -- I just pushed the code for establishing 2 separate Promise Chains. So, instead of doing this:

this._promise = createPromise()
outer._lastChild = this._promise.catch(()=> {})
// Oops!  Now `this.promise` will always think that its exceptions have been handled!

It is doing this:

const basePromise = createPromise();
this._promise = basePromise.then((x)=> x)
outer._lastChild = this._promise.catch(()=> {})
// Yay!  Now `this._promise` understands that its exceptions still need to be handled!

[briandamaged]

At a high-level, it seems like Knex is blurring the lines between:

1. The Transaction
2. The Transactor. (The thing that looks like a Knex Client, but it's actually scoped to a single transaction)
3. The operational logic around the Transactor. ("If the callback to knex.transaction(..) returns a Promise, then automatically commit or rollback the Transactor depending upon the outcome. Otherwise, assume that the caller will be responsible for committing/rolling back the Transactor")

In the current version of the Knex code, the Transaction class is responsible for instantiating the Transactor and handling the Transactor's operational logic as well. I suspect that we should extract these responsibilities into a higher layer of the code, such as here:

knex/lib/util/make-knex.js

Lines 39 to 50 in 7288608

	transaction(container, config) {
	const trx = this.client.transaction(container, config);
	trx.userParams = this.userParams;
	if (container) {
	return trx;
	}
	// If no container was passed, assume user wants to get a transaction and use it directly
	else {
	return trx.initPromise;
	}
	},

[maximelkin]

acquireConnection also has overrides in several dialects, check them please

[briandamaged]

The Transaction's acquireConnection method delegates to the underlying Dialect's acquireConnection method:

knex/lib/transaction.js

Lines 216 to 224 in 7288608

	acquireConnection(config, cb) {
	const configConnection = config && config.connection;
	return new Bluebird((resolve, reject) => {
	try {
	resolve(configConnection || this.client.acquireConnection());
	} catch (e) {
	reject(e);
	}
	})

So, there isn't actually an inheritance relationship there.

[briandamaged]

Actually, nvm. I see what you mean. I'll check it in a little bit.

(Related note: seems like there is a lot of duplication within those implementations)

[briandamaged]

So, the other implementations are actually ignoring the details around this._previousSibling. That is potentially a problem, but not a problem that was introduced by this specific change.

[briandamaged]

@maximelkin + @kibertoad : This current PR seems to fix the "unresolved Promise" issue that we were observing. However, I've actually done some additional cleanup/fixing as well:

https://github.com/briandamaged/knex/pull/1/files

This builds upon the existing PR and untangles the Transaction / operational logic. So, the Transaction class no longer needs to change its behavior depending upon how it was called. But, I'm keeping this as a separate PR for now just in case we want to merge things more incrementally.

[kibertoad]

This looks great, thanks a lot!

[kibertoad]

@briandamaged Can you also create the second PR?

[briandamaged]

@kibertoad : Sure!