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

Rework SQLCancel logistics to avoid possible races. #567

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ucko
Copy link
Contributor

@ucko ucko commented May 30, 2024

Split from #555.

@freddy77
Copy link
Contributor

freddy77 commented Jun 1, 2024

This smells like potential dead lock to me. In theory in Unix SQLCancel can be called inside a signal handler.

@ucko
Copy link
Contributor Author

ucko commented Jun 3, 2024

Good point. In that case, I suppose the way to go would be to set some flag on the DBC itself and check it reasonably often from other functions (perhaps by extending the macros around entry and exit); pretty much anything beyond that (even a trylock call) would be unsafe to call from a signal handler. :-/

* Split most of its logic into a new _SQLCancel; have SQLCancel itself
  merely set a flag indicating that an _SQLCancel call is in order.
* Automatically check that flag from ODBC_EXIT(_), ODBC_RETURN(_), and
  a new odbc_int_handler (which _SQLAllocEnv registers), by way
  of a new ODBC_CHECK_CANCEL macro.
* Check for cancellation on function entry too, bailing only when
  potentially seeking more results from a freshly canceled query; to
  that end, introduce ODBC_ENTER_HSTMT_OR_BAIL (and a helper
  ODBC_ENTER_HSTMT_EX).
* In _SQLCancel, rely on having the statement's mutex held and add
  HY008 (Operation was cancelled) after clearing other errors.

Signed-off-by: Aaron M. Ucko <[email protected]>
@ucko ucko force-pushed the ncbi-2024-05-SQLCancel-lock branch from 3d6f5f7 to f580b2a Compare June 13, 2024 17:56
@ucko ucko changed the title SQLCancel: Lock DBC mutex around extracting tds. Rework SQLCancel logistics to avoid possible races. Jun 13, 2024
@ucko
Copy link
Contributor Author

ucko commented Jun 13, 2024

I've substituted a rework along those lines that crucially also throws in an interrupt handler, and retitled this PR accordingly; the long commit description is

* Split most of its logic into a new _SQLCancel; have SQLCancel itself
  merely set a flag indicating that an _SQLCancel call is in order.
* Automatically check that flag from ODBC_EXIT(_), ODBC_RETURN(_), and
  a new odbc_int_handler (which _SQLAllocEnv registers), by way
  of a new ODBC_CHECK_CANCEL macro.
* Check for cancellation on function entry too, bailing only when
  potentially seeking more results from a freshly canceled query; to
  that end, introduce ODBC_ENTER_HSTMT_OR_BAIL (and a helper
  ODBC_ENTER_HSTMT_EX).
* In _SQLCancel, rely on having the statement's mutex held and add
  HY008 (Operation was cancelled) after clearing other errors.

@freddy77
Copy link
Contributor

freddy77 commented Jun 16, 2024

This last change looks overkilling. It reminds me of the hero movies in the 80-ish when the hero tried to pull out a bullet using a knife bigger than my forearm and trying not to hurt himself.
Joking aside it looks introducing more issues than what it's fixing. Surely needs more testing. But cancel have to do the cancel, not setting a flag and hoping for the best. Unfortunately too many cases are not tested, at least automatically. Should remove all data pending on same thread and statement? What the behaviour if connection is fully idle? What if the statement is not active? What if there are multiple active statement and you cancel one (MARS, obviously)?
Also I don't like having to poll every second.

@ucko
Copy link
Contributor Author

ucko commented Jun 17, 2024

I hear you, and had initially looked into finding ways to make immediate cancellation safe.

Alas, there are severe limits on what code potentially called from a signal handler can safely do. ISO is of course particularly strict; Windows is marginally better but still notably forbids any I/O. POSIX is more reasonable, but forbids any mutex operations, even trylock. In practice, temporarily masking signals around lock operations might let trylock (and corresponding unlock) calls succeed safely, but would add major overhead, and AFAICT isn't even an option on Windows (which is too strict anyway).

Incidentally, I see from reviewing that documentation that the flag should have type volatile sig_atomic_t to be fully safe.

@freddy77
Copy link
Contributor

Some working on cancellation, adding new tests... and finding new bugs!

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.

2 participants