You may have known that socket data is implemented as pool userdata in a manner that prevented multiple sockets from the same pool being able to use the same key for socket data. Another problem is that any cleanup specified in the call to apr_socket_data_set() is run when the pool is cleaned up, not when the socket is destroyed (in contradiction to the doc).
Err, yeah, why is it pool_userdata at all? I'm confused. Isn't using pool's userdata *way* overkill for this? Can't it just store a void* or something in its internal structure? Why does it have to live longer than the socket structure itself?
Are we really wanting to support multiple socket_data instances per socket? If a program wants that level, it seems it should just use the apr_pool_userdata directly rather than this unnecessary abstraction. KISS.
This patch fixes the first problem but not the second. But before attacking the second, I wonder if we really need cleanups on socket data. If a cleanup is really necessary, the application can make use subpools and pool cleanups in such a way as to achieve the goal.
Yeah, I think the cleanest thing is to throw-out the cleanups entirely. Rework it so it just does get/set of the void* (um, that's overkill - ooh, not because apr_socket_t is probably opaque). The cleanup can be done by whomever does the set call.
Some may even ask why we need socket data, since the application can achieve something equivalent by careful use of subpools and pool userdata.
I can buy the use of having a pointer associated with the socket. It's common enough, but yeah, you can do the same with pool_userdata directly. I'd prefer tossing out the key part in apr_socket_data_*, but if you want to be cute and backwards-compat, yeah, the hash works as you described. I just question how many apps really need that. -- justin
