Re: [zeromq-dev] libzmq - allocations, exceptions, alloc_assert and such
Sounds good to me. The extra complexity makes things more fragile IMO. On Wed, Jan 28, 2015 at 8:16 PM, Thomas Rodgers rodg...@twrodgers.com wrote: To understand correctly, we don't propagate such failures (as exceptions or error returns), and even if we did, it'd be pointless... My thinly veiled agenda here is - We are already toast in this case anyway, can we just get rid of the extra complexity that comes with trying pretend we can actually reliably do something under a memory exhaustion case? On Wed, Jan 28, 2015 at 1:07 PM, Pieter Hintjens p...@imatix.com wrote: To understand correctly, we don't propagate such failures (as exceptions or error returns), and even if we did, it'd be pointless... FWIW we added quite a lot handling in CZMQ to handle memory exhaustion. I don't particularly like that, as it makes things complex and it's unclear that there's any practical strategy for apps that hit such errors. On Tue, Jan 27, 2015 at 8:25 PM, Thomas Rodgers rodg...@twrodgers.com wrote: I know sustrik has previously railed on this topic, but at the risk of rehashing some of that ground... My reading of libzmq's source suggests that it goes to some lengths to prevent raising C++ exceptions, which, given that the C API functions do not attempt to guard against propagating exceptions to the caller, is a Good Thing. The code in some places uses malloc/free, in others either placement new into pre-allocated memory or new (std::nothrow) to prevent allocations from throwing. This is all well and good as far as it goes, but it doesn't go that far - stream_engine_t, mechanism_t, metadata_t, ctx_t (for example) all use node-based STL containers (std::map, std::mutlimap) which perform allocations on insertion/copy. mechanism_t's ctor default instantiates two std::mapstd::string, std::string's. Properties are then added to these dictionaries as they are consumed from the wire. Each property is stored as a key/value pair of std::string, which may throw on construction if it cannot allocate the requested space, and the insert may throw if it cannot allocate space for the map node. In stream_engine_t::mechanism_read() - It creates a local std::mapstd::string, std::string which it then merges properties exchanged from the transport mechanism (zap and zmtp) then constructs a metadata_t with a copy of that merged set. Though metadata_t is instantiated as follows - metadata = new (std::nothrow) metadata_t (properties); This can still throw. All std::nothrow guarantees is the allocation of space for metadata_t will not throw, it does not guard against metadata_t's ctor throwing (which can happen since it takes, and therefore must allocate space for, a copy of properties). This is perhaps somewhat academic since, in a low memory situation, you are likely hosed anyway (and on Linux you will likely never see an exception because of the OOM killer), but from a design standpoint the library currently has the ability to propagate errors back up the call stack to a client that can, in no way, do anything about them. Should the user facing API wrap and catch any propagated exceptions, and either return some error code or in the std::bad_alloc, do an alloc_assert and terminate? Another option that will address this case is to introduce a custom allocator which fails via alloc_assert, and update all STL type definitions to use that allocator. ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev
Re: [zeromq-dev] libzmq - allocations, exceptions, alloc_assert and such
To understand correctly, we don't propagate such failures (as exceptions or error returns), and even if we did, it'd be pointless... My thinly veiled agenda here is - We are already toast in this case anyway, can we just get rid of the extra complexity that comes with trying pretend we can actually reliably do something under a memory exhaustion case? On Wed, Jan 28, 2015 at 1:07 PM, Pieter Hintjens p...@imatix.com wrote: To understand correctly, we don't propagate such failures (as exceptions or error returns), and even if we did, it'd be pointless... FWIW we added quite a lot handling in CZMQ to handle memory exhaustion. I don't particularly like that, as it makes things complex and it's unclear that there's any practical strategy for apps that hit such errors. On Tue, Jan 27, 2015 at 8:25 PM, Thomas Rodgers rodg...@twrodgers.com wrote: I know sustrik has previously railed on this topic, but at the risk of rehashing some of that ground... My reading of libzmq's source suggests that it goes to some lengths to prevent raising C++ exceptions, which, given that the C API functions do not attempt to guard against propagating exceptions to the caller, is a Good Thing. The code in some places uses malloc/free, in others either placement new into pre-allocated memory or new (std::nothrow) to prevent allocations from throwing. This is all well and good as far as it goes, but it doesn't go that far - stream_engine_t, mechanism_t, metadata_t, ctx_t (for example) all use node-based STL containers (std::map, std::mutlimap) which perform allocations on insertion/copy. mechanism_t's ctor default instantiates two std::mapstd::string, std::string's. Properties are then added to these dictionaries as they are consumed from the wire. Each property is stored as a key/value pair of std::string, which may throw on construction if it cannot allocate the requested space, and the insert may throw if it cannot allocate space for the map node. In stream_engine_t::mechanism_read() - It creates a local std::mapstd::string, std::string which it then merges properties exchanged from the transport mechanism (zap and zmtp) then constructs a metadata_t with a copy of that merged set. Though metadata_t is instantiated as follows - metadata = new (std::nothrow) metadata_t (properties); This can still throw. All std::nothrow guarantees is the allocation of space for metadata_t will not throw, it does not guard against metadata_t's ctor throwing (which can happen since it takes, and therefore must allocate space for, a copy of properties). This is perhaps somewhat academic since, in a low memory situation, you are likely hosed anyway (and on Linux you will likely never see an exception because of the OOM killer), but from a design standpoint the library currently has the ability to propagate errors back up the call stack to a client that can, in no way, do anything about them. Should the user facing API wrap and catch any propagated exceptions, and either return some error code or in the std::bad_alloc, do an alloc_assert and terminate? Another option that will address this case is to introduce a custom allocator which fails via alloc_assert, and update all STL type definitions to use that allocator. ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev
Re: [zeromq-dev] libzmq - allocations, exceptions, alloc_assert and such
To understand correctly, we don't propagate such failures (as exceptions or error returns), and even if we did, it'd be pointless... FWIW we added quite a lot handling in CZMQ to handle memory exhaustion. I don't particularly like that, as it makes things complex and it's unclear that there's any practical strategy for apps that hit such errors. On Tue, Jan 27, 2015 at 8:25 PM, Thomas Rodgers rodg...@twrodgers.com wrote: I know sustrik has previously railed on this topic, but at the risk of rehashing some of that ground... My reading of libzmq's source suggests that it goes to some lengths to prevent raising C++ exceptions, which, given that the C API functions do not attempt to guard against propagating exceptions to the caller, is a Good Thing. The code in some places uses malloc/free, in others either placement new into pre-allocated memory or new (std::nothrow) to prevent allocations from throwing. This is all well and good as far as it goes, but it doesn't go that far - stream_engine_t, mechanism_t, metadata_t, ctx_t (for example) all use node-based STL containers (std::map, std::mutlimap) which perform allocations on insertion/copy. mechanism_t's ctor default instantiates two std::mapstd::string, std::string's. Properties are then added to these dictionaries as they are consumed from the wire. Each property is stored as a key/value pair of std::string, which may throw on construction if it cannot allocate the requested space, and the insert may throw if it cannot allocate space for the map node. In stream_engine_t::mechanism_read() - It creates a local std::mapstd::string, std::string which it then merges properties exchanged from the transport mechanism (zap and zmtp) then constructs a metadata_t with a copy of that merged set. Though metadata_t is instantiated as follows - metadata = new (std::nothrow) metadata_t (properties); This can still throw. All std::nothrow guarantees is the allocation of space for metadata_t will not throw, it does not guard against metadata_t's ctor throwing (which can happen since it takes, and therefore must allocate space for, a copy of properties). This is perhaps somewhat academic since, in a low memory situation, you are likely hosed anyway (and on Linux you will likely never see an exception because of the OOM killer), but from a design standpoint the library currently has the ability to propagate errors back up the call stack to a client that can, in no way, do anything about them. Should the user facing API wrap and catch any propagated exceptions, and either return some error code or in the std::bad_alloc, do an alloc_assert and terminate? Another option that will address this case is to introduce a custom allocator which fails via alloc_assert, and update all STL type definitions to use that allocator. ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev ___ zeromq-dev mailing list zeromq-dev@lists.zeromq.org http://lists.zeromq.org/mailman/listinfo/zeromq-dev
