per-module per-thread constructors and destructors
I need to add a thread local variable to rlm_otp. It's a per-thread fd which connects to another program running locally. rlm_otp makes requests of it and it returns results. The semantics of that program does not guarantee sequencing of results unless each request thread has its own connection. That is, if I have a global fd, the response received by one thread may not be to the correct request: t1 t2 get_state(foo) get_state(bar) -> write(foo) -> write(bar) -> read() -> read() -> bar -> foo The other program may return bar first, and even if it returned results in the order received, because both threads are blocked in read(), either might pick up the result for the other. An fd per thread fixes this. It also fixes the problem that writes() might interleave (atomic writes are only guaranteed in certain cases which won't be met here), but that is just icing for the cake. The problem, though, is that on thread exit no cleanup is done. I want to add two functions to threads.c: register_constructor() and register_destructor(). When a module is instantiated, it can call these to register functions to be called on thread creation and destruction. (rlm_otp will register a destructor which closes the fd.) It's fairly trivial, but it touches the "core" of freeradius, and it affects thread behavior, so I wanted to be sure there were no objections before adding it. -frank
Frank Cusack <fcusack@fcusack.com> wrote:
I need to add a thread local variable to rlm_otp.
I'm very, very, very, leery of adding thread-specific code to modules. It usually makes things much more complicated.
The other program may return bar first, and even if it returned results in the order received, because both threads are blocked in read(), either might pick up the result for the other. An fd per thread fixes this. It also fixes the problem that writes() might interleave (atomic writes are only guaranteed in certain cases which won't be met here), but that is just icing for the cake.
You could try using a connection pool, like LDAP & SQL. On initialization, open one connection. When the module is called, it looks for a free connection, and if none exists, creates one. As a larger problem, though, multiple modules need those kinds of pools, which means that the code should be abstracted into the server core. Alan DeKok.
On August 30, 2005 5:22:16 PM -0400 Alan DeKok <aland@ox.org> wrote:
Frank Cusack <fcusack@fcusack.com> wrote:
I need to add a thread local variable to rlm_otp.
I'm very, very, very, leery of adding thread-specific code to modules. It usually makes things much more complicated.
In this case, it makes it simpler. Handling a connection pool is much more complicated than simply adding ctor/dtor's.
The other program may return bar first, and even if it returned results in the order received, because both threads are blocked in read(), either might pick up the result for the other. An fd per thread fixes this. It also fixes the problem that writes() might interleave (atomic writes are only guaranteed in certain cases which won't be met here), but that is just icing for the cake.
You could try using a connection pool, like LDAP & SQL. On initialization, open one connection. When the module is called, it looks for a free connection, and if none exists, creates one.
OK, I looked at the LDAP module and it's unappealing. It's just overly complex for something that can be simple. (Similar to how losing init and destroy makes instantiate and detach more complex; although in that case it's not a big deal.) In fact, rlm_ldap gets it wrong. In ldap_get_conn(), conns[i].locked must not be tested until after a lock is obtained. rlm_sql seems to have a slight error as well, but it shouldn't manifest as a bug to the user. In sql_get_socket(), inst->last_used is not protected by a mutex. Oh, I see this is documented as acceptable. Both rlm_ldap and rlm_sql may have errors in connection teardown similar to the ones they have in setup. I didn't look. Note that connection pooling requires thread-specific code in the module as well. I'm going to go ahead and add ctor/dtor support. We can of course remove it if it looks too hairy. -frank
On August 30, 2005 3:07:33 PM -0700 Frank Cusack <fcusack@fcusack.com> wrote:
I'm going to go ahead and add ctor/dtor support. We can of course remove it if it looks too hairy.
Someone just pointed out pthread_key_create() to me, I'll use that instead. It's exactly what I wanted. (I was originally planning to use TLS [aka __thread] vars but they need additional destructor support which is what motivated my earlier email.) -frank
Frank Cusack <fcusack@fcusack.com> wrote:
Someone just pointed out pthread_key_create() to me, I'll use that instead. It's exactly what I wanted.
Sounds good. My $0.02, however, is that per-thread FD's probably aren't necessary. There may be 100's of threads in the server, each of which is in your module only a small amount of time. So 10 FD's may be good enough. Alan DeKok.
On August 30, 2005 7:08:55 PM -0400 Alan DeKok <aland@ox.org> wrote:
Frank Cusack <fcusack@fcusack.com> wrote:
Someone just pointed out pthread_key_create() to me, I'll use that instead. It's exactly what I wanted.
Sounds good.
My $0.02, however, is that per-thread FD's probably aren't necessary. There may be 100's of threads in the server, each of which is in your module only a small amount of time. So 10 FD's may be good enough.
Yeah, as I'm doing the implementation I'm tending to agree. We'll see what happens. -frank
Frank Cusack <fcusack@fcusack.com> wrote:
In ldap_get_conn(), conns[i].locked must not be tested until after a lock is obtained.
fixed in rlm_ldap.c#1.156 but I don't know how to propagate this change to 1.0.5 candidate.
Looking at the code, I'm not even sure that the "locked" member is appropriate for a threaded environment. It could probably be deleted. For non-threading mode, the LDAP module can't be using multiple connections, because it can only be called once. For threading mode, the mutexes protect the data structure. The "locked" member should be deleted, unless I'm missing something. Alan DeKok.
On August 30, 2005 6:42:39 PM -0400 Alan DeKok <aland@ox.org> wrote:
Frank Cusack <fcusack@fcusack.com> wrote:
In ldap_get_conn(), conns[i].locked must not be tested until after a lock is obtained.
fixed in rlm_ldap.c#1.156 but I don't know how to propagate this change to 1.0.5 candidate.
Looking at the code, I'm not even sure that the "locked" member is appropriate for a threaded environment. It could probably be deleted.
For non-threading mode, the LDAP module can't be using multiple connections, because it can only be called once. For threading mode, the mutexes protect the data structure.
The "locked" member should be deleted, unless I'm missing something.
sounds right to me. -frank
participants (2)
-
Alan DeKok -
Frank Cusack