Usefulness of mutex?

[sagotch]

Do you have a example where not using mutex would be a problem?
I do not understand what could go wrong if we remove this lock.

[tategakibunko]

Template context has some mutable resources(macro table is implemented by Hashtbl for examle).

So if it's shared by multiple thread, mutex lock is required.

But today I've read current source and now lock is enabled only while ast is calculated...!?

At the very first release, I've enabled mutex lock while whole output text is evaluated.

https://github.com/tategakibunko/jingoo/blob/v1.2.8/src/jg_interp.ml

But in current implementation, eval phase is not protected by mutex...

So I'll fix this part later.

1. Remove mutex lock code from ast_from_lexbuf
2. Guard from_file and from_string with mutex lock instead.

[tategakibunko]

Ahh, I've thought std_environment has some mutable code, and context is included in it, but environment value is transparent and context object is separated and always re-generated every time we parse, so mutex might not be required as you said ^^;

But in the future, some mutable resources might be included, so I want to remain current code.

Is it inconvenient for you? Or performance reason?

[sagotch]

I think that jingoo should be declared as not thread, so the user will use a mutex if he wants to use threads and the same context (but why would someone reuse the same context for 2 runs?).

A solution to ensure thread safety is to remove these Hashtbl and use Map instead. It require some refactoring but it could be the right thing to do.

My main problem with this is that I can not use jingoo with js_of_ocaml out of the box if I do not remove the mutex stuff (and also pcre, but I will propose a PR to replace it with re someday...). I removed it in my fork and I runs well in a single-page webapp (I'll share some code with you if you are interested in this project).

TL;DR; I do not think it worth it to embed threads and mutex into jingoo in order to implement thread safety. Either it should use immutable datastructures, or leave the thread safety management to user.

[tategakibunko]

the user will use a mutex, remove these Hashtbl and use Map instead.

OK, it seems right design we have to choose.

After we finish removing some mutable structure, I'll remove all mutex things from jingoo.

And replacing pcre to re sounds cool, because when I pushed new version of jingoo to opam-repository, some install errors were reported by pcre package in CI process, especially after official opam version updated to 2.0.

By the way, I'm really surprised that you are using jingoo in js project!

[tategakibunko]

To tell you the truth, I also tried to remove mutex things long ago, but some display trouble happend(but I don't remember details), It's because I'm so hesitant to change lock code.

Things I remember is that it was fcgi webapp(written by old ocamlnet) hosted by nginx(two threads fcgi settings), and version was jingoov1.2.12 or something.

In those days, displayed result was sometimes broken, so I restored lock code in hurry(without exploring the reason), and stopped touching it since then.

But today I removed lock code again and tested fcgi-app locally, nothing bad happens... so now I decided to remove lock code in next release, even if Hashtbl is not replaced by Map.

[sagotch]

If something goes wrong, the user can just add mutex where Jingoo is called, so removing it from jingoo's internal is a great choice imho. You may add a notice in the readme to warn that jingoo is not thread safe, but that's probably all you should care about 👍

[tategakibunko]

To users, we removed mutex locking.

7e5c526

Maybe it's not required, but if you want to confirm thread safety for your mind, please use mutex lock in your application code.