From e4305cf954c0039d6b4304ad562ce509f9172a7d Mon Sep 17 00:00:00 2001 From: David Rose Date: Sat, 5 Dec 2009 20:40:26 +0000 Subject: [PATCH] proper respect for locking with Python GIL, when true threads are enabled --- .../src/distributed/cConnectionRepository.cxx | 47 +++++++++++++-- .../src/distributed/cConnectionRepository.h | 60 +++++++++++-------- 2 files changed, 77 insertions(+), 30 deletions(-) diff --git a/direct/src/distributed/cConnectionRepository.cxx b/direct/src/distributed/cConnectionRepository.cxx index 6c38e5536b..20f734f5cf 100644 --- a/direct/src/distributed/cConnectionRepository.cxx +++ b/direct/src/distributed/cConnectionRepository.cxx @@ -296,7 +296,8 @@ check_datagram() { if(_native) _bdc.Flush(); #endif //WANT_NATIVE_NET - while (do_check_datagram()) { //Read a datagram + + while (do_check_datagram()) { if (get_verbose()) { describe_message(nout, "RECV", _dg); } @@ -319,9 +320,16 @@ check_datagram() { // structure, to support legacy code that expects to find it // there. if (_python_repository != (PyObject *)NULL) { +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_STATE gstate; + gstate = PyGILState_Ensure(); +#endif PyObject *value = PyLong_FromUnsignedLongLong(_msg_sender); PyObject_SetAttrString(_python_repository, "msgSender", value); Py_DECREF(value); +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif } #endif // HAVE_PYTHON } @@ -728,7 +736,12 @@ do_check_datagram() { //////////////////////////////////////////////////////////////////// bool CConnectionRepository:: handle_update_field() { - #ifdef HAVE_PYTHON +#ifdef HAVE_PYTHON +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_STATE gstate; + gstate = PyGILState_Ensure(); +#endif + PStatTimer timer(_update_pcollector); unsigned int do_id = _di.get_uint32(); if (_python_repository != (PyObject *)NULL) @@ -768,6 +781,9 @@ handle_update_field() { if (!cNeverDisable) { // in quiet zone and distobj is disable-able // drop update on the floor +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif return true; } } @@ -781,11 +797,18 @@ handle_update_field() { Py_DECREF(distobj); if (PyErr_Occurred()) { +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif return false; } } } + +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif #endif // HAVE_PYTHON return true; } @@ -804,7 +827,12 @@ handle_update_field() { //////////////////////////////////////////////////////////////////// bool CConnectionRepository:: handle_update_field_owner() { - #ifdef HAVE_PYTHON +#ifdef HAVE_PYTHON +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_STATE gstate; + gstate = PyGILState_Ensure(); +#endif + PStatTimer timer(_update_pcollector); unsigned int do_id = _di.get_uint32(); if (_python_repository != (PyObject *)NULL) { @@ -855,6 +883,9 @@ handle_update_field_owner() { Py_DECREF(distobjOV); if (PyErr_Occurred()) { +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif return false; } } @@ -891,13 +922,19 @@ handle_update_field_owner() { Py_DECREF(distobj); if (PyErr_Occurred()) { +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif return false; } } } - } - #endif // HAVE_PYTHON + +#if defined(HAVE_THREADS) && !defined(SIMPLE_THREADS) + PyGILState_Release(gstate); +#endif +#endif // HAVE_PYTHON return true; } diff --git a/direct/src/distributed/cConnectionRepository.h b/direct/src/distributed/cConnectionRepository.h index c2023e92ff..aa2aa254f3 100644 --- a/direct/src/distributed/cConnectionRepository.h +++ b/direct/src/distributed/cConnectionRepository.h @@ -64,6 +64,16 @@ PUBLISHED: bool threaded_net = false); ~CConnectionRepository(); + // Any methods of this class that acquire _lock (which is most of + // them) *must* be tagged BLOCKING, to avoid risk of a race + // condition in Python when running in true threaded mode. The + // BLOCKING tag releases the Python GIL during the function call, + // and we re-acquire it when needed within these functions to call + // out to Python. If any functions acquire _lock while already + // holding the Python GIL, there could be a deadlock between these + // functions and the ones that are acquiring the GIL while already + // holding _lock. + INLINE DCFile &get_dc_file(); INLINE bool has_owner_view() const; @@ -85,25 +95,25 @@ PUBLISHED: #endif #ifdef HAVE_OPENSSL - void set_connection_http(HTTPChannel *channel); - SocketStream *get_stream(); + BLOCKING void set_connection_http(HTTPChannel *channel); + BLOCKING SocketStream *get_stream(); #endif #ifdef HAVE_NET - bool try_connect_net(const URLSpec &url); - + BLOCKING bool try_connect_net(const URLSpec &url); + INLINE QueuedConnectionManager &get_qcm(); INLINE ConnectionWriter &get_cw(); INLINE QueuedConnectionReader &get_qcr(); #endif #ifdef WANT_NATIVE_NET - bool connect_native(const URLSpec &url); + BLOCKING bool connect_native(const URLSpec &url); INLINE Buffered_DatagramConnection &get_bdc(); #endif #ifdef SIMULATE_NETWORK_DELAY - void start_delay(double min_delay, double max_delay); - void stop_delay(); + BLOCKING void start_delay(double min_delay, double max_delay); + BLOCKING void stop_delay(); #endif BLOCKING bool check_datagram(); @@ -114,37 +124,37 @@ PUBLISHED: #endif #endif - INLINE void get_datagram(Datagram &dg); - INLINE void get_datagram_iterator(DatagramIterator &di); - INLINE CHANNEL_TYPE get_msg_channel(int offset = 0) const; - INLINE int get_msg_channel_count() const; - INLINE CHANNEL_TYPE get_msg_sender() const; + BLOCKING INLINE void get_datagram(Datagram &dg); + BLOCKING INLINE void get_datagram_iterator(DatagramIterator &di); + BLOCKING INLINE CHANNEL_TYPE get_msg_channel(int offset = 0) const; + BLOCKING INLINE int get_msg_channel_count() const; + BLOCKING INLINE CHANNEL_TYPE get_msg_sender() const; // INLINE unsigned char get_sec_code() const; - INLINE unsigned int get_msg_type() const; + BLOCKING INLINE unsigned int get_msg_type() const; INLINE static const string &get_overflow_event_name(); - bool is_connected(); + BLOCKING bool is_connected(); BLOCKING bool send_datagram(const Datagram &dg); - INLINE void set_want_message_bundling(bool flag); - INLINE bool get_want_message_bundling() const; + BLOCKING INLINE void set_want_message_bundling(bool flag); + BLOCKING INLINE bool get_want_message_bundling() const; - INLINE void set_in_quiet_zone(bool flag); - INLINE bool get_in_quiet_zone() const; + BLOCKING INLINE void set_in_quiet_zone(bool flag); + BLOCKING INLINE bool get_in_quiet_zone() const; - void start_message_bundle(); - INLINE bool is_bundling_messages() const; - void send_message_bundle(unsigned int channel, unsigned int sender_channel); - void abandon_message_bundles(); - void bundle_msg(const Datagram &dg); + BLOCKING void start_message_bundle(); + BLOCKING INLINE bool is_bundling_messages() const; + BLOCKING void send_message_bundle(unsigned int channel, unsigned int sender_channel); + BLOCKING void abandon_message_bundles(); + BLOCKING void bundle_msg(const Datagram &dg); BLOCKING bool consider_flush(); BLOCKING bool flush(); - void disconnect(); - void shutdown(); + BLOCKING void disconnect(); + BLOCKING void shutdown(); INLINE void set_simulated_disconnect(bool simulated_disconnect); INLINE bool get_simulated_disconnect() const;