Skip to content
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 an API for off-thread SQL transactions. #26

Merged
merged 14 commits into from
Jun 7, 2014
Merged

Add an API for off-thread SQL transactions. #26

merged 14 commits into from
Jun 7, 2014

Conversation

dvander
Copy link
Member

@dvander dvander commented Jun 5, 2014

No description provided.

@KyleSanderson
Copy link
Member

Forgive me in advance (not really sure of an elegant way to reference lines).

  1. It looks like a plugin can clone a transaction handle to retain access after execution.
  2. In TTransactOp::CallSuccess if the database handle cannot be allocated, it looks like the results are leaked? Making a generic cleanup function might be a path. ( https://github.com/alliedmodders/sourcemod/pull/26/files#diff-f5d9dd1d22aa85b23963361c04db78cfR1588 )
  3. super-nit: mixed spaces/tabs in the include file.

Regardless, I've definitely lost data as a result of not using transactions: this is great.

@Impact123
Copy link
Contributor

Some parts of the code seem to ignore the Coding style guidelines, i'm a little bit confused about that.

@dvander
Copy link
Member Author

dvander commented Jun 5, 2014

@KyleSanderson Thanks, fixed the spacing issues (sort of). I'm trying to avoid using hard-tabs for alignment, an old mistake we made early on.

re: cloning. I forget most of how HandleSys security restrictions work, but trying to folllow it - CloneHandle() should pass owner=plugin, and the transaction type is created with HANDLE_RESTRICT_OWNER|HANDLE_RESTRICT_IDENTITY. So in theory in the owner check should fail. I added a test to confirm.

Edit: whoops, it works because CloneHandle passes ident=NULL. So RESTRICT_IDENTITY apparently is enough.

@Impact123 The SM style has evolved over time as we re-evaluate readability - those changes haven't been reflected in that document. I'll try to update it this week.

@KyleSanderson
Copy link
Member

:shipit:

dvander added a commit that referenced this pull request Jun 7, 2014
Add an API for off-thread SQL transactions. (bug 3775, r=kyle)
@dvander dvander merged commit 6ac897f into master Jun 7, 2014
@dvander dvander deleted the sql-txn branch June 7, 2014 03:51
This was referenced Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants