On Wed, Feb 25, 2015 at 12:48 PM, Ted Ross <[email protected]> wrote: > > > On 02/25/2015 11:52 AM, Rafael Schloming wrote: > >> On Wed, Feb 25, 2015 at 10:49 AM, Ted Ross <[email protected]> wrote: >> >> Would it be safe to assume that any operations on driver->io are not >>> thread safe? >>> >>> Dispatch is a multi-threaded application. It looks to me as though >>> io->error is a resource shared across the threads in an unsafe way. >>> >>> >> Interesting... so this is what the docs say: >> >> /** >> * A ::pn_io_t manages IO for a group of pn_socket_t handles. A >> * pn_io_t object may have zero or one pn_selector_t selectors >> * associated with it (see ::pn_io_selector()). If one is associated, >> * all the pn_socket_t handles managed by a pn_io_t must use that >> * pn_selector_t instance. >> * >> * The pn_io_t interface is single-threaded. All methods are intended >> * to be used by one thread at a time, except that multiple threads >> * may use: >> * >> * ::pn_write() >> * ::pn_send() >> * ::pn_recv() >> * ::pn_close() >> * ::pn_selector_select() >> * >> * provided at most one thread is calling ::pn_selector_select() and >> * the other threads are operating on separate pn_socket_t handles. >> */ >> > > I claim that the commit-in-question violates the text above. Calls to > pn_send() and pn_recv() are no longer thread-safe because they now use the > shared error record. >
You could be right. I'm not entirely sure how to interpret the above text. I don't know that I would necessarily consider pn_send/pn_recv to be "methods" of pn_io_t. > >> I think this has been somewhat modified by the constraints from the >> windows >> implementation, and I'm not sure I understand completely what the >> constraints are there, or entirely what is being described above, but on >> the posix front, the pn_io_t is little more than just a holder for an >> error >> slot, and you should have one of these per thread. It shouldn't be a >> problem to use send/recv/etc from multiple threads though so long as you >> pass in the pn_io_t from the current thread. >> > > It's not desirable to allocate sockets to threads up front (i.e. partition > the set of sockets into per-thread slots). I know you didn't say that was > needed but it's what I infer from the docs for pn_io_t. > I don't think the posix implementation requires this. > Assuming, as you suggest, that pn_io_t is nothing more than a > thread-specific error notepad seems like a recipe for future disaster > because pn_io_t is clearly intended to be more than that. > It may not work out so well on windows, I honestly don't know what the situation is there, but certainly for posix systems I think we need *something* in this area to function as a context that can be associated with thread-or-smaller granularities. Having some way to return error information is just one example of a useful context to be able to use at thread or smaller granularities. If the windows I/O APIs require a heavier-weight interface, then perhaps we need to factor it into two different parts. --Rafael
