On May 14, 2009, at 12:37 AM, Chris Darroch wrote:
Hi -- So, define AP_SLOTMEM_PROVIDER_GROUP as "slotmem" and AP_SLOTMEM_PROVIDER_VERSION as "0". Remove AP_SLOTMEM_STORAGE. Rename mod_sharedmem to mod_slotmem_shm and mod_plainmem to mod_slotmem_plain. Rename ap_slotmem_storage_method to ap_slotmem_provider_t. Rename ap_slotmem_t to ap_slotmem_instance_t. Remove the "slotmem_" prefix from method names (i.e., the names of the functions provided by a provider). If I've got a slotmem provider, it's just extra typing to have to call slotmem->slotmem_create() instead of slotmem->create().
Assuming we remove the abstraction, +1.
Next, there are several problems I see with the apslotmem_type enum at the moment. For one thing, there's only one enumerated value, SLOTMEM_PERSIST, so there's no way to specify "don't persist". I hacked around that by passing 1 in mod_shmap. Also, if we retain this type, I'd suggest renaming it to a slightly more conventional looking ap_slotmem_type_t. But my personal preference would be to remove it and instead add an unsigned int flags field to ap_slotmem_provider_t, and define some flag values, such as AP_SLOTMEM_FLAG_NOTMPSAFE = 0x0001 (read "not MP-safe") and AP_SLOTMEM_FLAG_PERSIST = 0x0002. While enumerated types are clean if you've got a long list of mutually exclusive values, a flags field follows the existing socache framework, and allows for a number of mutually non-exclusive flags to be set. This is actually closer to what I suspect future development might bring.
Again, +1 for this change.
At any rate, moving responsibility for locking up to the caller level, as the socache API does, I think makes a lot of sense. It means that a caller running in a single-process, single-threaded context can simply choose not to add the overhead of a global lock. Other callers can make their own decisions about what kind of locking they need.
In general, I think that a provider should know if it is thread-safe or not and simply "work" without the user needing to know or worry. I think if the provider knows that threads are available AND the MPM is threaded, it should by default mutex as required instead of having users all need to recreate that externally. If the user says "I don't care what MPM I have, don't worry about locking", then I am +1 on a flag of some time telling the provider to bypass that. Even so, then, I think that we should expose lock/unlock if we make this a user-land "you decide if you want to do it or not"... I don't think it makes sense having the user/caller have to worry about re-inventing the wheel as far as mutexes are concerned just to use a provider... A provider should provide the full set of functions the end-user should need in order to use it. (imo)
