Opened 9 years ago

Closed 9 years ago

#8916 closed Bugs (fixed)

boost/signals2/detail/lwm_pthreads.hpp: Ignores all failures from pthread_* functions

Reported by: Jeffrey Walton <noloader@…> Owned by: Frank Mori Hess
Milestone: To Be Determined Component: signals2
Version: Boost 1.54.0 Severity: Problem
Keywords: signals2 mutex pthread API failure Cc:

Description

boost/signals2/detail/lwm_pthreads.hpp ignores all failures from pthread_* functions. Functions include pthread_mutex_init, pthread_mutex_lock, pthread_mutex_unlock and pthread_mutex_destroy.

A lock failure is usually a bad thing, and I can't come up with scenarios where a silent failure is desired. It will make a bad problem worse by corrupting data or terminating the program.

At minimum (as a user), I would expect for Boost to use BOOST_ASSERT with an appropriate exception in debugging and diagnostic builds; and BOOST_VERIFY with an appropriate exception for release or production builds.

Perhaps it would be a good idea to use boost/thread/pthread/mutex.hpp or boost/interprocess/sync/posix/mutex.hpp. They appear to be more mature and have a bit more stability. In addition, it throws lock exceptions where appropriate.

Change History (5)

comment:1 by Frank Mori Hess, 9 years ago

(In [85161]) Added BOOST_VERIFY checks on return values from pthread calls. Refs #8916

comment:2 by Frank Mori Hess, 9 years ago

You are free to use boost::mutex with signals2 signals if you choose, it's just not the default. This is because when I wrote it, boost::mutex was not header-only, and apparently still is not.

comment:3 by Jeffrey Walton <noloader@…>, 9 years ago

Added BOOST_VERIFY checks on return values from pthread calls

As I understand it (please correct me), BOOST_VERIFY is simply BOOST_ASSERT without the side effect of removal when NDEBUG is defined [1]. That is:

  • with NDEBUG, BOOST_ASSERT(x) is removed (including x)
  • without NDEBUG, BOOST_ASSERT(x) calls assert(x)

For BOOST_VERIFY:

  • with NDEBUG, BOOST_VERIFY(x) is simply x
  • without NDEBUG, BOOST_VERIFY(x) is BOOST_ASSERT(x)

The best that happens in an assert for Diagnostic or Debug builds (i.e., NDEBUG is not defined); but no throw on failure. And in Production or Release code (i.e., NDEBUG is defined), then nothing happens.

[1] http://www.boost.org/doc/libs/1_54_0/libs/utility/assert.html

in reply to:  3 comment:4 by Frank Mori Hess, 9 years ago

Replying to Jeffrey Walton <noloader@…>:

As I understand it (please correct me), BOOST_VERIFY is simply BOOST_ASSERT without the side effect of removal when NDEBUG is defined [1]. That is:

That's right. I'm guessing you're arguing that is not sufficient? According to the man pages, it is impossible for the pthread functions to return errors as I am using them, except by programmer error. Thus asserting seems good enough. This is also what boost.smart_ptr does (which is where I cribbed the mutex code from). Below is a paste of the pthread man page I'm referring to. I'm not using an error checking mutex, and I am initializing the mutex, so the only possible non-zero return value is if the mutex is destroyed while locked.

RETURN VALUE

pthread_mutex_init always returns 0. The other mutex functions return 0 on success and a non-zero error code on error.

ERRORS

The pthread_mutex_lock function returns the following error code on error:

EINVAL the mutex has not been properly initialized.

EDEADLK

the mutex is already locked by the calling thread (error checking mutexes only).

The pthread_mutex_trylock function returns the following error codes on error:

EBUSY the mutex could not be acquired because it was currently locked.

EINVAL the mutex has not been properly initialized.

The pthread_mutex_unlock function returns the following error code on error:

EINVAL the mutex has not been properly initialized.

EPERM the calling thread does not own the mutex (error checking mutexes only).

The pthread_mutex_destroy function returns the following error code on error:

EBUSY the mutex is currently locked.

comment:5 by Frank Mori Hess, 9 years ago

Resolution: fixed
Status: newclosed

(In [85211]) Merged from trunk. Closes #8916

Note: See TracTickets for help on using tickets.