Release lock during io #29

brendanlong · 2018-09-26T01:16:53Z

No description provided.

brendanlong · 2018-09-26T01:17:25Z

I'm not sure why it wouldn't let me re-open #27 but this is the same thing but with code to track whether or not we have the runtime lock, which makes it much easier to verify that we're doing things right in the callbacks.

This passes the tests in this repo and also my company's tests (which are unfortunately based on a higher-level library so I can't easily merge them into this repo), so I think it's ready to merge if you're happy with it. Let me know if you want any changes.

brendanlong · 2018-09-26T01:25:43Z

Actually one more issue.. the error and message handlers can be called by dbopen, which is before we have our lock tracker setup.

brendanlong · 2018-09-26T01:33:37Z

Ok it should be good now. I added special handling for the dbopen case and a new test_bad_login test.

brendanlong · 2018-09-26T01:36:07Z

src/dblib_stubs.c

+  if (had_lock) {
+    release_lock(dbproc);
+  }
+  CAMLreturn(0);


Note: This used to have a comment /* should not return */, but this function will return as long as the callback doesn't throw an exception. Also the Sybase docs for dbmsghandle say:

The message handler must return a value of 0 to DB-Library.

http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.ocs_12.5.1.dblib/html/dblib/X12458.htm

Oh, and it's not entirely clear to me if it's safe to call CAMLreturn without the lock, but i don't know how to trigger CAMLreturn's cleanups otherwise?

brendanlong · 2018-09-26T01:37:05Z

src/dblib_stubs.c

+  if (had_lock) {
+    release_lock(dbproc);
+  }
+  CAMLreturn(INT_CANCEL);


Another case where this can return if the error handle doesn't throw an exception.

Chris00 · 2018-09-26T06:47:08Z

Makefile

@@ -1,7 +1,7 @@
 all: build

 build:
-	@jbuilder build @install @test --dev
+	@jbuilder build @install @test


--dev is now implicit in dune. Will make the explicit switch to dune so you can rebase on it.

Yeah I was getting this error with the --dev:

dune: --dev is no longer accepted as it is now the default. Usage: dune build [OPTION]... [TARGET]... Try `dune build --help' or `dune --help' for more information. make: *** [Makefile:4: build] Error 1

I added this as pull request #28 if you want to review that on its own.

I just pushed a full port to Dune (and the included configurator). Could you rebase on top if it?

(Remove your changes to the Makefile and use dune instead of jbuild.)

I just rebased it

Chris00 · 2018-09-26T16:56:00Z

May you remind me: the library does not accept several concurrent calls in the same program?

brendanlong · 2018-09-26T17:39:15Z

May you remind me: the library does not accept several concurrent calls in the same program?

http://www.freetds.org/faq.html#thread.safe

Different threads may all use separate connections without interfering with each other. Threads may not share a DBPROCESS or CS_CONNECTION without controlling access via a mutex.

So we can use multiple FreeTDS connections in different threads, we just can't use the same connection in multiple threads.

brendanlong · 2018-09-26T17:56:52Z

Hm I don't understand the new build failures.. Maybe it's related to the Dune change?

brendanlong · 2018-09-26T17:57:59Z

Ah yeah, the CI build failures seem to be from the previous commit, not this one.

It does build and pass tests locally so I think there's just something wrong with the CI builds.

Chris00 · 2018-09-27T12:15:21Z

Travis is fixed. For AppVeyor, there remains a bug in opam.

Chris00 · 2018-09-27T12:15:40Z

May you please rebase?

brendanlong · 2018-09-27T14:16:57Z

Rebased

brendanlong · 2018-09-27T18:32:21Z

Hm just ran into a segfault in code that seems possibly related to this.. I'm going to check into it more. This is really complex to get right :(

Chris00 · 2018-09-28T16:52:39Z

src/dblib_stubs.c


 /* OCaml severity.  Keep in sync with OCaml [Dblib]. */
 #define SEVERITY_PROGRAM Val_int(6)
 #define SEVERITY_RESOURCE Val_int(7)
 #define SEVERITY_FATAL Val_int(9)
 #define SEVERITY_CONSISTENCY Val_int(10)

+static int release_lock(DBPROCESS *dbproc)


Wouldn't freetds_release_runtime_system be clearer?

Makes sense

Chris00 · 2018-09-28T16:53:45Z

src/dblib_stubs.c

+  int *have_lock = (int*)dbgetuserdata(dbproc);
+
+  // If have_lock wasn't setup, then we're in the msg/error callback for dbopen
+  if (have_lock == NULL) {


Shouldn't the lock be allocated & initialized when the dbproc value is created (and released by its finalizer)?

That's what this pull request does. The problem is that dbopen both allocates a dbproc and attempts to connect to the remote server, so if the connection fails, we'll have the exception handler called before we have a chance to call dbsetuserdata.

brendanlong · 2018-09-28T18:43:25Z

src/dblib_stubs.c

+  server = String_val(vserver);
+  caml_release_runtime_system();
+  dbproc = dbopen(login, server);
+  caml_acquire_runtime_system();


@Chris00 This is the case where we need to handle have_runtime_lock being null, since we don't want to hold the lock while we're making the initial connection to the server, but we don't have a (non-null) dbproc until line 302.

This releases OCaml's runtime lock whenever we call C functions that do IO. It should allow users of this to run ocaml-freetds in parallel, or when using Async/Lwt. I had to re-arrange some code to avoid calling any OCaml functions while the lock was released. We use wrappers around the release/acquire functions to ensure that we don't try to acquire or release the lock twice, which could potentially happen in the msg/error callbacks.

brendanlong · 2018-09-28T20:13:52Z

Ok I figured out what I was causing problems in our system. I had the check at the end of the message handler backwards:

  if (had_lock) {
    freetds_release_runtime_system(dbproc);
  }

Should have been:

  if (!had_lock) {
    freetds_release_runtime_system(dbproc);
  }

That's fixed and now it's working well in our systems, so this can be merged if you're happy with it.

I also renamed the functions as requested.

Note that I added another commit to add (name) to the dune-project file, which was necessary to make opam pin work.

Chris00 · 2018-09-28T23:12:02Z

Thanks fort working on this. Unfortunately, I won't have time to do a thorough review until next Tuesday. I'll merge or make additional comments then.

brendanlong · 2018-10-22T19:47:51Z

We're seeing some random inexplicable syntax errors that may be related to this change (syntax error on character not in the query), although it's hard to tell since it occurs at random. Our guess right now is that either FreeTDS is not as thread-safe as it claims to be (supposedly it should be safe to use different connections concurrently, it's just that individual connection handles aren't thread safe), or the way we throw exceptions out of C code in the callbacks is doing bad things to FreeTDS's internals.

I'd like to fix this library to never throw exeptions in C callbacks but it's kind of hard.

I'm also looking into re-implementing TDS from scratch in https://github.com/brendanlong/ocaml-tds

Chris00 · 2018-10-22T21:08:36Z

Did you ask on the freeTDS mailing list?

brendanlong · 2018-11-08T00:04:29Z

We're investigating a different approach, trying to use the Ctlib backend instead since it doesn't have callbacks.

brendanlong force-pushed the release-lock-during-io branch 2 times, most recently from d071311 to ea0878a Compare September 26, 2018 01:32

brendanlong commented Sep 26, 2018

View reviewed changes

Chris00 reviewed Sep 26, 2018

View reviewed changes

brendanlong force-pushed the release-lock-during-io branch from ea0878a to 7f8dfd0 Compare September 26, 2018 16:28

Chris00 force-pushed the master branch 2 times, most recently from 0135dd7 to aa28e2b Compare September 27, 2018 11:02

brendanlong force-pushed the release-lock-during-io branch from 7f8dfd0 to 3f37311 Compare September 27, 2018 13:51

Chris00 reviewed Sep 28, 2018

View reviewed changes

brendanlong commented Sep 28, 2018

View reviewed changes

Add (name) to dune-project

9c364b6

brendanlong force-pushed the release-lock-during-io branch from 3f37311 to 0be4014 Compare September 28, 2018 20:11

brendanlong force-pushed the release-lock-during-io branch from 0be4014 to d18f320 Compare September 28, 2018 20:12

brendanlong mentioned this pull request Oct 22, 2018

Inexplicable syntax errors (threading related?) arenadotio/ocaml-mssql#5

Closed

brendanlong closed this Nov 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release lock during io #29

Release lock during io #29

brendanlong commented Sep 26, 2018

brendanlong commented Sep 26, 2018 •

edited

Loading

brendanlong commented Sep 26, 2018

brendanlong commented Sep 26, 2018

brendanlong Sep 26, 2018

brendanlong Sep 26, 2018

brendanlong Sep 26, 2018

Chris00 Sep 26, 2018

brendanlong Sep 26, 2018 •

edited

Loading

Chris00 Sep 26, 2018

Chris00 Sep 26, 2018

brendanlong Sep 26, 2018

Chris00 commented Sep 26, 2018

brendanlong commented Sep 26, 2018

brendanlong commented Sep 26, 2018 •

edited

Loading

brendanlong commented Sep 26, 2018

Chris00 commented Sep 27, 2018

Chris00 commented Sep 27, 2018

brendanlong commented Sep 27, 2018

brendanlong commented Sep 27, 2018

Chris00 Sep 28, 2018

brendanlong Sep 28, 2018

Chris00 Sep 28, 2018

brendanlong Sep 28, 2018

brendanlong Sep 28, 2018

brendanlong commented Sep 28, 2018 •

edited

Loading

Chris00 commented Sep 28, 2018

brendanlong commented Oct 22, 2018

Chris00 commented Oct 22, 2018

brendanlong commented Nov 8, 2018

Release lock during io #29

Release lock during io #29

Conversation

brendanlong commented Sep 26, 2018

brendanlong commented Sep 26, 2018 • edited Loading

brendanlong commented Sep 26, 2018

brendanlong commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanlong Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris00 commented Sep 26, 2018

brendanlong commented Sep 26, 2018

brendanlong commented Sep 26, 2018 • edited Loading

brendanlong commented Sep 26, 2018

Chris00 commented Sep 27, 2018

Chris00 commented Sep 27, 2018

brendanlong commented Sep 27, 2018

brendanlong commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanlong commented Sep 28, 2018 • edited Loading

Chris00 commented Sep 28, 2018

brendanlong commented Oct 22, 2018

Chris00 commented Oct 22, 2018

brendanlong commented Nov 8, 2018

brendanlong commented Sep 26, 2018 •

edited

Loading

brendanlong Sep 26, 2018 •

edited

Loading

brendanlong commented Sep 26, 2018 •

edited

Loading

brendanlong commented Sep 28, 2018 •

edited

Loading