stop function is thread safe

[saghul]

Either make all of them thread safe or none of them. A mixed solution is Bad (TM). IMHO only AE.call_from_thread should be thread safe, if users want to stop the loop from another thread they need to use that, and if they don't consequences are unforeseen.

[ibc]

Yeah, I get you. However making AE.stop thread safe could avoid "issues" in people not properly reading the documentation (imagine they just want to use other thread to stop AE... ugly but...).

I prefer to leave AE.stop thread safe, but I should make a decision regardless what the future doc must say: should it say that AE.stop is thread safe? or not?

IMHO the doc could clearly say:

• For adding/modifying/stopping handles from other thread you MUST pass a block to AE.call_from_other_thread.
• In addition, AE.stop is thread safe by itself.

PS: There is another thread safe method: AE.run. Even if AE is already running you can invoke, at any time and from any thread, AE.run { block }. This will run the block within the existing loop (or will call AE.call_from_other_thread(block) if AE is running in a different thread).

[saghul]

You create the rules, so don't create exceptions to the rules you are creating. It's very simple to say "ONLY AE.call_from_thread is thread safe". Nice and simple :-)

As for the AE.run thing, it should also NOT be thread safe. Moreover, if AE is already running it should raise and exception, not run any block, there is next_tick for that. You can have a mutex lock to protect the _running flag, so if any thread wants to do AE.run you get the lock, check if _running is True, and if it is, raise some AlreadyRunningError.

[ibc]

My rationale for allowing multiple AE.run calls and leaving it thread safe along with AE.stop is the following:

Different libraries/gems could internally use AE, without the developer expecting that the user code would also invoke AE at the same time. Therefore, forcing the developer (or the user) to check AE.running? and if so running next_tick (or call_from_other_thread !!!) rather than a simple AE.run would be difficult to achieve.

[saghul]

A library should never call AE.run, the application should do that. IMHO you are trying to make a one-fits-all solution which will byte you in the future ;-)

[ibc]

Yes, you are right. However allowing AE.run and AE.stop to be re-entrant and thread safe are about 4 lines of code, it breaks nothing and does not make unfeasible any possible feature.

BTW take a look to some test units:

• https://github.com/ibc/AsyncEngine/blob/master/test/test_basic.rb#L69
• https://github.com/ibc/AsyncEngine/blob/master/test/test_basic.rb#L94
• https://github.com/ibc/AsyncEngine/blob/master/test/test_basic.rb#L103
• https://github.com/ibc/AsyncEngine/blob/master/test/test_threads.rb#L23
• https://github.com/ibc/AsyncEngine/blob/master/test/test_threads.rb#L47

[saghul]

Calling AE.run twice should always be an error. It's simple and understandable. Same goes for stop, if you try to stop a already stopped AE you should get an exception. You'll need to keep another flag which will be set to true between you call stop and the stop actually happens. Something like '_stopping'.

[ibc]

AE.stop returns false if AE was not running. Raising an exception is dangerous, i.e. in multithread environments in which different threads could attemp to terminate the reactor at same time due to some satisfied condition (it's the very same if they use AE.call_from_other_thread { AE.stop }).

About AE.run being re-entrant (is this what people call "re-entrant"???), EM does it so I must do, and not because EM does it, but because I consider it "could" be useful in some cases. If the developer really does not want to enter AE.run if AE was already running, he can always check AE.running? method before calling to AE.run. I see no problem.

[saghul]

Your project, your choices :-) I don't like everything about Twisted, both Twisted and Tornado act as I said, and Twisted has been around for over 10 years, and I really think it's the right thing to do.

Instead of making AE.stop raise an exception you could make it silent by returning false if you want, though I prefer exceptions. The caller should try-except if he knows he is calling stop from different threads, which he shouldn't do in the first place.

Also, testing for running and calling run later is not thread safe, another thread may have started AE in between.

Please, don't implement things because someone "could" abuse them, build them invariably correct.

[ibc]

This is not exactly what you expected but hope it helps: 7de5ee0

[saghul]

Well, it's your project, so what can I say? :-) I'd not do it like that.

[ibc]

Ok, let me advance. I promise you to reconsider this subject (really) once I've implemented TCP, DNS, fd integration, filesystem operations and... TCP-TLS ! (you know) and... UDP-DTLS?

[saghul]

I'll come back, you'll not get away that easy ;-)