Details
-
Test
-
Status: Closed
-
Major
-
Resolution: Fixed
-
1.16.1
-
None
Description
Analysis in DISPATCH-2183 shows that PYTHON and SERVER locks are taken in both orders and could cause a deadlock.
Attachments
Activity
codecov-commenter commented on pull request #1295:
URL: https://github.com/apache/qpid-dispatch/pull/1295#issuecomment-877513015
- [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging 1295(https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (604769d) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/6d546780da043b72959d6fa65004402e254c75cd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d54678) will *increase* coverage by `0.02%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
-
- main #1295 +/- ##
==========================================
+ Coverage 84.61% 84.64% +0.02%
==========================================
Files 113 113
Lines 28234 28235 +1
==========================================
+ Hits 23891 23899 +8
+ Misses 4343 4336 -7
```
- main #1295 +/- ##
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> *Legend* - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d54678...604769d](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
–
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
jiridanek commented on a change in pull request #1295:
URL: https://github.com/apache/qpid-dispatch/pull/1295#discussion_r668054841
##########
File path: src/server.c
##########
@@ -919,12 +919,12 @@ static void qd_connection_free(qd_connection_t *qd_conn)
sys_mutex_lock(qd_server->lock);
DEQ_REMOVE(qd_server->conn_list, qd_conn);
+ sys_mutex_unlock(qd_server->lock);
// If counted for policy enforcement, notify it has closed
if (qd_conn->policy_counted) {
qd_policy_socket_close(qd_server->qd->policy, qd_conn);
Review comment:
For easier reference, TSan is not happy with `qd_policy_socket_close`
https://github.com/apache/qpid-dispatch/pull/1295/checks?check_run_id=3032721509#step:25:1109
https://github.com/apache/qpid-dispatch/pull/1295/checks?check_run_id=3032721509#step:25:2139
```
21: E WARNING: ThreadSanitizer: data race (pid=1862)
21: E Write of size 8 at 0x000000528f08 by thread T4 (mutexes: write M308):
21: E #0 qd_policy_socket_close /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/policy.c:291 (qdrouterd+0x47fd04)
21: E #1 qd_connection_free /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/server.c:926 (qdrouterd+0x4def8f)
21: E #2 thread_run /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/server.c:1127 (qdrouterd+0x4e3d5b)
21: E #3 _thread_init /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/posix/threading.c:172 (qdrouterd+0x48667d)
21: E
21: E Previous read of size 8 at 0x000000528f08 by thread T3:
21: E #0 qd_policy_socket_close /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/policy.c:318 (qdrouterd+0x47fe79)
21: E #1 qd_connection_free /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/server.c:926 (qdrouterd+0x4def8f)
21: E #2 thread_run /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/server.c:1127 (qdrouterd+0x4e3d5b)
21: E #3 _thread_init /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/posix/threading.c:172 (qdrouterd+0x48667d)
21: E
21: E Location is global 'n_connections' of size 8 at 0x000000528f08 (qdrouterd+0x000000528f08)
21: E
21: E Mutex M308 (0x7b1000000e80) created at:
21: E #0 pthread_mutex_init <null> (libtsan.so.0+0x49603)
21: E #1 sys_mutex /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/posix/threading.c:43 (qdrouterd+0x4866cc)
21: E #2 qd_policy /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/policy.c:124 (qdrouterd+0x47f352)
21: E #3 qd_dispatch_prepare /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/dispatch.c:338 (qdrouterd+0x4661cf)
21: E #4 ffi_call_unix64 <null> (libffi.so.6+0x6c03)
21: E #5 main_process /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/router/src/main.c:97 (qdrouterd+0x426c6c)
21: E #6 main /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/router/src/main.c:369 (qdrouterd+0x42622c)
21: E
21: E Thread T4 (tid=1872, running) created by main thread at:
21: E #0 pthread_create <null> (libtsan.so.0+0x5bf45)
21: E #1 sys_thread /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/posix/threading.c:181 (qdrouterd+0x486b0c)
21: E #2 qd_server_run /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/server.c:1499 (qdrouterd+0x4e490c)
21: E #3 main_process /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/router/src/main.c:115 (qdrouterd+0x426ccc)
21: E #4 main /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/router/src/main.c:369 (qdrouterd+0x42622c)
21: E
21: E Thread T3 (tid=1871, running) created by main thread at:
21: E #0 pthread_create <null> (libtsan.so.0+0x5bf45)
21: E #1 sys_thread /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/posix/threading.c:181 (qdrouterd+0x486b0c)
21: E #2 qd_server_run /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/server.c:1499 (qdrouterd+0x4e490c)
21: E #3 main_process /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/router/src/main.c:115 (qdrouterd+0x426ccc)
21: E #4 main /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/router/src/main.c:369 (qdrouterd+0x42622c)
21: E
21: E SUMMARY: ThreadSanitizer: data race /home/runner/work/qpid-dispatch/qpid-dispatch/qpid-dispatch/src/policy.c:291 in qd_policy_socket_close
21: E ==================
21: E ThreadSanitizer: reported 1 warnings
21: E <<<<
```
–
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
codecov-commenter edited a comment on pull request #1295:
URL: https://github.com/apache/qpid-dispatch/pull/1295#issuecomment-877513015
- [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging 1295(https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9e72664) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/6d546780da043b72959d6fa65004402e254c75cd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d54678) will *increase* coverage by `0.10%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
-
- main #1295 +/- ##
==========================================
+ Coverage 84.61% 84.72% +0.10%
==========================================
Files 113 113
Lines 28234 28246 +12
==========================================
+ Hits 23891 23932 +41
+ Misses 4343 4314 -29
```
- main #1295 +/- ##
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> *Legend* - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d54678...9e72664](https://codecov.io/gh/apache/qpid-dispatch/pull/1295?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
–
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Commit e90df062b08fb572e5d865c69feb0f20dd202abe in qpid-dispatch's branch refs/heads/main from Charles E. Rolke
[ https://gitbox.apache.org/repos/asf?p=qpid-dispatch.git;h=e90df06 ]
DISPATCH-2195: Fix SERVER-PYTHON lock inversion
- Release server lock before calling qd_policy_socket_close instead
of after.
- As a general rule the PYTHON lock must not be taken while holding
any other lock.
- Make a copy of the global while holding the lock and then log the
value of the copy.
This closes #1295
asfgit closed pull request #1295:
URL: https://github.com/apache/qpid-dispatch/pull/1295
–
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
ChugR opened a new pull request #1295:
URL: https://github.com/apache/qpid-dispatch/pull/1295
Release server lock before calling qd_policy_socket_close instead
of after.
As a general rule the PYTHON lock must not be taken while holding
any other lock.
–
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org