Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConflictDetector: kill both tasks #2540

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

smurfix
Copy link
Contributor

@smurfix smurfix commented Jan 18, 2023

It's not always obvious which task is the "bad guy" when a conflict occurs. If the "legitimate" task crashes and the buggy one, i.e. the one that forgot to take the lock, does not, the backtrace is not helpful.

Thus, kill them both.

It's not always obvious which task is the "bad guy" when a conflict
occurs. If the "legitimate" task crashes and the buggy one, i.e. the one
that forgot to take the lock, does not, the backtrace is not helpful.

Thus, kill them both.
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #2540 (6adbaf0) into master (82533e8) will decrease coverage by 1.07%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2540      +/-   ##
==========================================
- Coverage   93.50%   92.44%   -1.07%     
==========================================
  Files         118      118              
  Lines       16335    16340       +5     
  Branches     3156     3157       +1     
==========================================
- Hits        15274    15105     -169     
- Misses        955     1105     +150     
- Partials      106      130      +24     
Impacted Files Coverage Δ
trio/_util.py 94.91% <100.00%> (+0.22%) ⬆️
trio/testing/_check_streams.py 100.00% <100.00%> (ø)
trio/tests/test_signals.py 100.00% <100.00%> (ø)
trio/tests/test_ssl.py 99.86% <100.00%> (ø)
trio/tests/test_testing.py 99.53% <100.00%> (ø)
trio/tests/test_util.py 100.00% <100.00%> (ø)
trio/_subprocess_platform/kqueue.py 0.00% <0.00%> (-100.00%) ⬇️
trio/_core/_io_kqueue.py 0.80% <0.00%> (-83.88%) ⬇️
trio/lowlevel.py 58.33% <0.00%> (-16.67%) ⬇️
trio/_core/__init__.py 75.00% <0.00%> (-12.50%) ⬇️
... and 14 more

@smurfix smurfix marked this pull request as draft January 18, 2023 20:17
@smurfix smurfix requested a review from pquentin January 18, 2023 20:17
@smurfix smurfix force-pushed the conflict branch 2 times, most recently from ecc3f5e to 4857c1b Compare January 18, 2023 20:27
@smurfix smurfix marked this pull request as ready for review January 18, 2023 20:54
@smurfix
Copy link
Contributor Author

smurfix commented Jan 18, 2023

These codecov results seem rather spurious.

@oremanj
Copy link
Member

oremanj commented Jan 18, 2023

This is a breaking change. Previously, if you wanted to see if some other task was waiting on an FD, you could try to wait on it and catch BusyResourceError. I have used this pattern to implement "make sure someone is waiting" on multiplexed interfaces (such as inotify) without a system task. I would be frustrated if we broke that.

@pquentin pquentin removed their request for review February 13, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants