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

C-threads and callbacks #4702

Closed
vicuna opened this issue Jan 25, 2009 · 10 comments
Closed

C-threads and callbacks #4702

vicuna opened this issue Jan 25, 2009 · 10 comments
Assignees

Comments

@vicuna
Copy link

vicuna commented Jan 25, 2009

Original bug ID: 4702
Reporter: db
Assigned to: @xavierleroy
Status: closed (set by @xavierleroy on 2012-09-25T18:06:17Z)
Resolution: fixed
Priority: normal
Severity: feature
Version: 3.11.0
Fixed in version: 3.12.1+dev
Category: ~DO NOT USE (was: OCaml general)
Related to: #8236 #6764 #7220
Monitored by: braibant @ygrek @dra27 @Chris00 @alainfrisch @mmottl

Bug description

I am hacking Systhreads library to make possible Ocaml callbacks in externally created C-threads (according to #8236 and http://groups.google.ru/group/fa.caml/browse_thread/thread/b2629058ac0de613). My idea is to add two functions:

CAMLextern void caml_c_thread_register(void);
CAMLextern void caml_c_thread_unregister(void);

and then use them like this:

/* register the current thread in Ocaml (once) /
caml_c_thread_register(void);
...
caml_leave_blocking_section();
caml_callback(...);
caml_enter_blocking_section();
...
/
unregister the current thread before its completion */
caml_c_thread_unregister(void);

That works for me now (under Win32 yet but pthreads are also not a problem) but caml_c_thread_register() does DuplicateHandle() inside that theoretically can lead to an error. What is the preferable way to handle errors in Ocaml<->C interface functions? Once this become clear I can send you a patch (if you are interested).

File attachments

@vicuna
Copy link
Author

vicuna commented Mar 28, 2009

Comment author: @xavierleroy

This looks like a very reasonable approach. At some point I considered doing the thread registration within caml_leave_blocking_section, when it sees a thread that wasn't created from Caml, but I didn't know how to do the un-registration automatically at thread exit. The API you propose is cleaner overall. A patch will be welcome if you have completed an implementation already.

Concerning error reporting, a reasonable rule of thumb is that a C interface function intended to be called from Caml raises an exception, while a function intended to be called from C either returns an error code, and it is the caller's responsibility to handle it or ignore it. So, caml_c_thread_register() should probably return an error code. The funny thing is that the Windows tradition is to return 1 on success and 0 on error, while the Unix tradition is 0 on success and a non-zero code on error. I'll let you choose the style you prefer!

@vicuna
Copy link
Author

vicuna commented Mar 30, 2009

Comment author: db

OK, the Windows implementation is ready and attached. It works for me for several months. It also makes possible unloading a DLL with the embedded threaded Ocaml runtime (unpatched Ocaml crashes as the tick thread is not terminated and sigterm handler is not restored).

As for POSIX implementation - there are some issues. Does the similar fix (stopping the tick thread and restoring the handlers) is required for anyone? Then an additional synchronization for the tick thread is needed (brute-force Win32 approach won't work). And currently I have no established Linux environment where I could test the (future) patch. So it will take some time.

@vicuna
Copy link
Author

vicuna commented Apr 2, 2010

Comment author: db

There was a bug in my implementation; I will post an update this weekend.

@vicuna
Copy link
Author

vicuna commented Apr 2, 2010

Comment author: @xavierleroy

FYI, I started to merge your previous patch and adapt it to POSIX threads, and ended up refactoring a lot of code between the Win32- and POSIX-specific files. The refactored code is not yet in the SVN trunk because we haven't yet decided to include it in 3.12.0, but can be found in the branch branches/newsysthreads. So maybe you want to have a look at the refactored implementation before investing more time in your patch. I'd be interested to know about the bug you found in your original implementation. Regards,

-Xavier Leroy

@vicuna
Copy link
Author

vicuna commented Apr 5, 2010

Comment author: db

Well, it maybe not entirely the bug but a feature I needed for correct work of my Ocaml module.

The following scenario was triggering the problem:

  1. A C process creates a separate thread, loads Ocaml DLL and initializes Ocaml runtime.
  2. Some processing is done in that thread.
  3. The thread is terminated on C side. caml_c_thread_unregister() should be called. No Ocaml thread exists now. curr_thread should be NULL.

The problem was that after (3) curr_thread actually contained a garbage, so Ocaml runtime crashed then another C thread was created and caml_c_thread_register() was called. And some special care should be taken to not caml_thread_scan_roots() when no Ocaml thread existed.

If you are interested in that usage pattern to work correctly I will check out the new implementation and post the necessary changes.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2010

Comment author: @xavierleroy

Merged branches/newsysthreads in the SVN trunk, will go in 3.12.0.
Rewrote caml_thread_{un,}register so that they do not depend on curr_thread.
More testing is needed, both for the new registration stuff and for the refactored library in general.

@vicuna
Copy link
Author

vicuna commented May 29, 2011

Comment author: db

Sorry for not reporting this in time, but it would be great if you do systhreads cleanup at Ocaml runtime termination. Otherwise DLL-embedded Ocaml would crash after DLL unloading. I will attach the patch if it's possible for a reopened issue.

@vicuna
Copy link
Author

vicuna commented Jun 4, 2011

Comment author: @xavierleroy

Dmitry, please send the patch (if you can't attach it, e-mail it to Xavier.Leroy AT inria.fr). I'm afraid you're the only one affected by the problem, so it won't be fixed without your help. Thanks.

@vicuna
Copy link
Author

vicuna commented Jun 5, 2011

Comment author: db

I have already attached it to this issue (systhreads.diff). If something is wrong with it, please let me know.

@vicuna
Copy link
Author

vicuna commented Jun 6, 2011

Comment author: @xavierleroy

Thanks for the patch (I didn't see it was already there). Applied in 3.12 bugfix branch; will go in 3.12.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants