Skip to content

Always set _closed future in StdoutWriterProtocol.connection_lost #24

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Always set _closed future in StdoutWriterProtocol.connection_lost #24

wants to merge 3 commits into from

Conversation

HW42
Copy link

@HW42 HW42 commented May 14, 2025 •
edited
Loading

Otherwise when the stdout stream is closed wait_closed will hang forever.

Also add tests that check that the service terminates correctly.

And add a fix of #25 for Ubuntu 22.04.

QubesOS/qubes-issues#9779

@HW42 HW42 mentioned this pull request May 14, 2025
Open
@marmarek
Copy link
Member

PipelineRetry

@marmarek
Copy link
Member

Python < 3.12 used to have a bug in wait_closed() that it didn't actually waited for all connections. f488ef1 looks relevant.

@codecov Codecov
Copy link

codecov bot commented May 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.13725% with 7 lines in your changes missing coverage. Please review.

Project coverage is 80.49%. Comparing base (47259d4) to head (45a239d).

Files with missing lines Patch % Lines
splitgpg2/stdiostream.py 0.00% 6 Missing ⚠️
splitgpg2/test_termination.py 98.95% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   79.20%   80.49%   +1.29%     
==========================================
  Files           3        4       +1     
  Lines        1433     1528      +95     
==========================================
+ Hits         1135     1230      +95     
  Misses        298      298

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HW42
Copy link
Author

HW42 commented May 14, 2025

Python < 3.12 used to have a bug in wait_closed() that it didn't actually waited for all connections. f488ef1 looks relevant.

Yup. I did mention that commit already in QubesOS/qubes-issues#9779 (comment).

f488ef1 added the wait_close call and 2eb10ac made it actually wait instead of raising NotImplementedError

@qubesos-bot qubesos-bot mentioned this pull request May 30, 2025
Draft
@qubesos-bot
Copy link

qubesos-bot commented May 30, 2025 •
edited
Loading

OpenQA test summary

Complete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025060520-4.3&flavor=pull-requests

Test run included the following:

New failures, excluding unstable

Compared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025031804-4.3&flavor=update

  • system_tests_splitgpg

  • system_tests_extra

    • TC_10_Thunderbird_whonix-workstation-17: test_000_send_receive_default (failure)
      dogtail.tree.SearchError: descendent of [application | Thunderbird]...

    • TC_00_QVCTest_whonix-gateway-17: test_010_screenshare (failure)
      ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^... AssertionError: 0 == 0

    • TC_00_QVCTest_whonix-workstation-17: test_010_screenshare (failure)
      AssertionError: 1 != 0 : Timeout waiting for /dev/video0 in test-in...

  • system_tests_dispvm

  • system_tests_kde_gui_interactive

    • gui_keyboard_layout: wait_serial (wait serial expected)
      # wait_serial expected: "echo -e '[Layout]\nLayoutList=us,de' | sud...

    • gui_keyboard_layout: Failed (test died)
      # Test died: command 'test "$(cd ~user;ls e1*)" = "$(qvm-run -p wor...

  • system_tests_qwt_win10_seamless@hw13

    • windows_clipboard_and_filecopy: unnamed test (unknown)
    • windows_clipboard_and_filecopy: Failed (test died)
      # Test died: no candidate needle with tag(s) 'windows-Edge-address-...

  • system_tests_basic_vm_qrexec_gui_ext4

    • switch_pool: Failed (test died)
      # Test died: command 'migrate_templates' failed at /usr/lib/os-auto...

Failed tests

17 failures

  • system_tests_splitgpg

  • system_tests_extra

    • TC_10_Thunderbird_whonix-workstation-17: test_000_send_receive_default (failure)
      dogtail.tree.SearchError: descendent of [application | Thunderbird]...

    • TC_00_QVCTest_whonix-gateway-17: test_010_screenshare (failure)
      ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^... AssertionError: 0 == 0

    • TC_00_QVCTest_whonix-workstation-17: test_010_screenshare (failure)
      AssertionError: 1 != 0 : Timeout waiting for /dev/video0 in test-in...

  • system_tests_dispvm

  • system_tests_kde_gui_interactive

    • gui_keyboard_layout: wait_serial (wait serial expected)
      # wait_serial expected: "echo -e '[Layout]\nLayoutList=us,de' | sud...

    • gui_keyboard_layout: Failed (test died)
      # Test died: command 'test "$(cd ~user;ls e1*)" = "$(qvm-run -p wor...

  • system_tests_qwt_win10_seamless@hw13

    • windows_clipboard_and_filecopy: unnamed test (unknown)
    • windows_clipboard_and_filecopy: Failed (test died)
      # Test died: no candidate needle with tag(s) 'windows-Edge-address-...

  • system_tests_basic_vm_qrexec_gui_ext4

    • switch_pool: Failed (test died)
      # Test died: command 'migrate_templates' failed at /usr/lib/os-auto...

Fixed failures

Compared to: https://openqa.qubes-os.org/tests/132953#dependencies

14 fixed

  • system_tests_whonix

    • whonixcheck: fail (unknown)
      Whonixcheck for sys-whonix failed...

    • whonixcheck: unnamed test (unknown)

  • system_tests_suspend

    • suspend: unnamed test (unknown)
    • suspend: Failed (test died)
      # Test died: no candidate needle with tag(s) 'SUSPEND-FAILED' match...

  • system_tests_basic_vm_qrexec_gui

  • system_tests_qrexec

  • system_tests_kde_gui_interactive

    • clipboard_and_web: unnamed test (unknown)

    • clipboard_and_web: Failed (test died)
      # Test died: no candidate needle with tag(s) 'qubes-website' matche...

    • clipboard_and_web: wait_serial (wait serial expected)
      # wait_serial expected: "lspci; echo 2E8vz-\$?-"...

  • system_tests_guivm_vnc_gui_interactive

    • gui_filecopy: unnamed test (unknown)
    • gui_filecopy: Failed (test died)
      # Test died: no candidate needle with tag(s) 'files-work' matched...

  • system_tests_audio

  • system_tests_whonix@hw7

    • whonixcheck: fail (unknown)
      Whonixcheck for sys-whonix failed...

    • whonixcheck: unnamed test (unknown)

Unstable tests

Performance Tests

Performance degradation:

15 performance degradations
  • debian-12-xfce_exec: 8.27 🔺 ( previous job: 7.12, degradation: 116.20%)
  • debian-12-xfce_exec-data-duplex-root: 91.61 🔺 ( previous job: 82.72, degradation: 110.75%)
  • dom0_root_seq1m_q8t1_read 3:read_bandwidth_kb: 385789.00 :small_red_triangle: ( previous job: 446963.00, degradation: 86.31%)
  • dom0_root_seq1m_q1t1_read 3:read_bandwidth_kb: 74102.00 :small_red_triangle: ( previous job: 294295.00, degradation: 25.18%)
  • dom0_root_seq1m_q1t1_write 3:write_bandwidth_kb: 24664.00 :small_red_triangle: ( previous job: 95454.00, degradation: 25.84%)
  • dom0_root_rnd4k_q32t1_read 3:read_bandwidth_kb: 15405.00 :small_red_triangle: ( previous job: 79803.00, degradation: 19.30%)
  • dom0_root_rnd4k_q32t1_write 3:write_bandwidth_kb: 1295.00 :small_red_triangle: ( previous job: 6149.00, degradation: 21.06%)
  • dom0_root_rnd4k_q1t1_write 3:write_bandwidth_kb: 426.00 :small_red_triangle: ( previous job: 4826.00, degradation: 8.83%)
  • dom0_varlibqubes_seq1m_q8t1_write 3:write_bandwidth_kb: 100351.00 :small_red_triangle: ( previous job: 250795.00, degradation: 40.01%)
  • fedora-41-xfce_root_seq1m_q1t1_write 3:write_bandwidth_kb: 49359.00 :small_red_triangle: ( previous job: 87940.00, degradation: 56.13%)
  • fedora-41-xfce_root_rnd4k_q32t1_write 3:write_bandwidth_kb: 2902.00 :small_red_triangle: ( previous job: 3599.00, degradation: 80.63%)
  • fedora-41-xfce_private_seq1m_q1t1_read 3:read_bandwidth_kb: 280143.00 :small_red_triangle: ( previous job: 334687.00, degradation: 83.70%)
  • fedora-41-xfce_private_rnd4k_q1t1_write 3:write_bandwidth_kb: 691.00 :small_red_triangle: ( previous job: 1130.00, degradation: 61.15%)
  • fedora-41-xfce_volatile_seq1m_q8t1_write 3:write_bandwidth_kb: 112412.00 :small_red_triangle: ( previous job: 179949.00, degradation: 62.47%)
  • fedora-41-xfce_volatile_rnd4k_q32t1_write 3:write_bandwidth_kb: 3591.00 :small_red_triangle: ( previous job: 5672.00, degradation: 63.31%)

Remaining performance tests:

57 tests
  • debian-12-xfce_exec-root: 28.73 🔺 ( previous job: 28.65, degradation: 100.26%)
  • debian-12-xfce_socket: 8.81 🔺 ( previous job: 8.60, degradation: 102.43%)
  • debian-12-xfce_socket-root: 8.07 🟢 ( previous job: 8.52, improvement: 94.66%)
  • debian-12-xfce_exec-data-simplex: 76.17 🔺 ( previous job: 71.62, degradation: 106.35%)
  • debian-12-xfce_exec-data-duplex: 64.23 🟢 ( previous job: 70.34, improvement: 91.31%)
  • debian-12-xfce_socket-data-duplex: 157.41 🔺 ( previous job: 156.96, degradation: 100.29%)
  • fedora-41-xfce_exec: 9.43 🔺 ( previous job: 9.27, degradation: 101.72%)
  • fedora-41-xfce_exec-root: 58.51 🟢 ( previous job: 61.51, improvement: 95.12%)
  • fedora-41-xfce_socket: 9.01 🔺 ( previous job: 8.63, degradation: 104.39%)
  • fedora-41-xfce_socket-root: 9.07 🔺 ( previous job: 8.71, degradation: 104.18%)
  • fedora-41-xfce_exec-data-simplex: 64.14 🟢 ( previous job: 75.53, improvement: 84.92%)
  • fedora-41-xfce_exec-data-duplex: 76.84 🔺 ( previous job: 71.56, degradation: 107.38%)
  • fedora-41-xfce_exec-data-duplex-root: 96.07 🟢 ( previous job: 109.13, improvement: 88.04%)
  • fedora-41-xfce_socket-data-duplex: 137.08 🟢 ( previous job: 150.61, improvement: 91.02%)
  • whonix-gateway-17_exec: 6.89 🔺 ( previous job: 6.82, degradation: 101.04%)
  • whonix-gateway-17_exec-root: 40.50 🔺 ( previous job: 40.43, degradation: 100.17%)
  • whonix-gateway-17_socket: 7.54 🔺 ( previous job: 7.24, degradation: 104.15%)
  • whonix-gateway-17_socket-root: 8.02 🔺 ( previous job: 7.65, degradation: 104.83%)
  • whonix-gateway-17_exec-data-simplex: 78.03 🟢 ( previous job: 78.32, improvement: 99.63%)
  • whonix-gateway-17_exec-data-duplex: 77.79 🔺 ( previous job: 76.65, degradation: 101.49%)
  • whonix-gateway-17_exec-data-duplex-root: 87.43 🟢 ( previous job: 88.52, improvement: 98.76%)
  • whonix-gateway-17_socket-data-duplex: 168.56 🟢 ( previous job: 171.76, improvement: 98.14%)
  • whonix-workstation-17_exec: 8.38 🔺 ( previous job: 7.67, degradation: 109.21%)
  • whonix-workstation-17_exec-root: 59.09 🔺 ( previous job: 58.26, degradation: 101.41%)
  • whonix-workstation-17_socket: 8.25 🔺 ( previous job: 8.19, degradation: 100.68%)
  • whonix-workstation-17_socket-root: 8.86 🔺 ( previous job: 8.13, degradation: 109.03%)
  • whonix-workstation-17_exec-data-simplex: 74.45 🟢 ( previous job: 74.99, improvement: 99.27%)
  • whonix-workstation-17_exec-data-duplex: 77.06 🔺 ( previous job: 72.71, degradation: 105.99%)
  • whonix-workstation-17_exec-data-duplex-root: 100.80 🔺 ( previous job: 99.82, degradation: 100.98%)
  • whonix-workstation-17_socket-data-duplex: 159.15 🟢 ( previous job: 169.50, improvement: 93.89%)
  • dom0_root_seq1m_q8t1_write 3:write_bandwidth_kb: 138589.00 :green_circle: ( previous job: 129298.00, improvement: 107.19%)
  • dom0_root_rnd4k_q1t1_read 3:read_bandwidth_kb: 11203.00 :green_circle: ( previous job: 10795.00, improvement: 103.78%)
  • dom0_varlibqubes_seq1m_q8t1_read 3:read_bandwidth_kb: 365867.00 :small_red_triangle: ( previous job: 382273.00, degradation: 95.71%)
  • dom0_varlibqubes_seq1m_q1t1_read 3:read_bandwidth_kb: 435817.00 :small_red_triangle: ( previous job: 437636.00, degradation: 99.58%)
  • dom0_varlibqubes_seq1m_q1t1_write 3:write_bandwidth_kb: 181858.00 :small_red_triangle: ( previous job: 184752.00, degradation: 98.43%)
  • dom0_varlibqubes_rnd4k_q32t1_read 3:read_bandwidth_kb: 103673.00 :green_circle: ( previous job: 62195.00, improvement: 166.69%)
  • dom0_varlibqubes_rnd4k_q32t1_write 3:write_bandwidth_kb: 8126.00 :green_circle: ( previous job: 6479.00, improvement: 125.42%)
  • dom0_varlibqubes_rnd4k_q1t1_read 3:read_bandwidth_kb: 8026.00 :green_circle: ( previous job: 7669.00, improvement: 104.66%)
  • dom0_varlibqubes_rnd4k_q1t1_write 3:write_bandwidth_kb: 4484.00 :small_red_triangle: ( previous job: 4903.00, degradation: 91.45%)
  • fedora-41-xfce_root_seq1m_q8t1_read 3:read_bandwidth_kb: 379643.00 :green_circle: ( previous job: 368309.00, improvement: 103.08%)
  • fedora-41-xfce_root_seq1m_q8t1_write 3:write_bandwidth_kb: 190255.00 :green_circle: ( previous job: 162081.00, improvement: 117.38%)
  • fedora-41-xfce_root_seq1m_q1t1_read 3:read_bandwidth_kb: 288307.00 :small_red_triangle: ( previous job: 318716.00, degradation: 90.46%)
  • fedora-41-xfce_root_rnd4k_q32t1_read 3:read_bandwidth_kb: 85391.00 :green_circle: ( previous job: 82694.00, improvement: 103.26%)
  • fedora-41-xfce_root_rnd4k_q1t1_read 3:read_bandwidth_kb: 8516.00 :green_circle: ( previous job: 8485.00, improvement: 100.37%)
  • fedora-41-xfce_root_rnd4k_q1t1_write 3:write_bandwidth_kb: 1328.00 :green_circle: ( previous job: 542.00, improvement: 245.02%)
  • fedora-41-xfce_private_seq1m_q8t1_read 3:read_bandwidth_kb: 386500.00 :green_circle: ( previous job: 373957.00, improvement: 103.35%)
  • fedora-41-xfce_private_seq1m_q8t1_write 3:write_bandwidth_kb: 181618.00 :green_circle: ( previous job: 170062.00, improvement: 106.80%)
  • fedora-41-xfce_private_seq1m_q1t1_write 3:write_bandwidth_kb: 60619.00 :small_red_triangle: ( previous job: 61534.00, degradation: 98.51%)
  • fedora-41-xfce_private_rnd4k_q32t1_read 3:read_bandwidth_kb: 88411.00 :green_circle: ( previous job: 80283.00, improvement: 110.12%)
  • fedora-41-xfce_private_rnd4k_q32t1_write 3:write_bandwidth_kb: 3134.00 :green_circle: ( previous job: 2215.00, improvement: 141.49%)
  • fedora-41-xfce_private_rnd4k_q1t1_read 3:read_bandwidth_kb: 7764.00 :green_circle: ( previous job: 7540.00, improvement: 102.97%)
  • fedora-41-xfce_volatile_seq1m_q8t1_read 3:read_bandwidth_kb: 350811.00 :small_red_triangle: ( previous job: 369868.00, degradation: 94.85%)
  • fedora-41-xfce_volatile_seq1m_q1t1_read 3:read_bandwidth_kb: 299850.00 :small_red_triangle: ( previous job: 324737.00, degradation: 92.34%)
  • fedora-41-xfce_volatile_seq1m_q1t1_write 3:write_bandwidth_kb: 56230.00 :green_circle: ( previous job: 17567.00, improvement: 320.09%)
  • fedora-41-xfce_volatile_rnd4k_q32t1_read 3:read_bandwidth_kb: 80462.00 :green_circle: ( previous job: 79021.00, improvement: 101.82%)
  • fedora-41-xfce_volatile_rnd4k_q1t1_read 3:read_bandwidth_kb: 8680.00 :green_circle: ( previous job: 7867.00, improvement: 110.33%)
  • fedora-41-xfce_volatile_rnd4k_q1t1_write 3:write_bandwidth_kb: 2137.00 :green_circle: ( previous job: 1953.00, improvement: 109.42%)

This was referenced May 30, 2025
Open
Open
Open
Merged
Open
Open
Merged
Draft
Merged
Draft
Open
@marmarek
Copy link
Member

marmarek commented Jun 1, 2025

I took a bit deeper look at this, and first of all, this fix (as in - always setting _closed future in connection_lost) looks correct to me.
A side effect of this change is changing the order of handling self._drain_waiters and self._closed. In practice, it doesn't really matter, as none of the waiting functions on those would execute in the meantime (connection_lost is not async def). But in theory, it should be first waking up drain waiters and only then closed, so better move drain waiters loop to be run under if self._paused: (instead of after if not self._paused: return).

This was referenced Jun 1, 2025
Merged
Open
Open
Open
@HW42
Copy link
Author

HW42 commented Jun 4, 2025 •
edited
Loading

Thanks for taking a closer look at this.

A side effect of this change is changing the order of handling self._drain_waiters and self._closed. [...]

Good point. I misread that logic initially (Based on the comment I thought it wakes up the blocking writer, but actually it tests for not self._paused and else wakes up the drain waiters). With that your suggestion makes sense.

Do you understand why that if is there at all? Shouldn't self._drain_waiters always be empty if not self._paused?

@marmarek
Copy link
Member

marmarek commented Jun 4, 2025

Do you understand why that if is there at all? Shouldn't self._drain_waiters always be empty if not self._paused?

Not really, it was this way in asyncio.FlowControlMixin already. Looking at git history there, it used to be a single waiter, not a list. But still, it's not clear to me what that if was added back then, the commit message doesn't help much... (it survived a few refactors, initially was added here: python/cpython@355491d)

@HW42
Always set _closed future in StdoutWriterProtocol.connection_lost
ce3d443 
Otherwise when the stdout stream is closed wait_closed will hang
forvever.

See QubesOS#24 for
discussion about the exact intended behavior or WriterProtocol in this
case.

QubesOS/qubes-issues#9779
@HW42 HW42 force-pushed the simon/fix-exit branch 3 times, most recently from 98d2086 to 16c94c6 Compare June 5, 2025 13:00
@HW42 HW42 force-pushed the simon/fix-exit branch from 16c94c6 to d7e75a3 Compare June 5, 2025 13:36
@HW42
Fix Python invocation on Ubuntu 22.04 (jammy)
45a239d 
It doesn't support -P yet. Work around it by changing the directory do
/. split-gpg2 shouldn't depend on it.
@HW42 HW42 force-pushed the simon/fix-exit branch from 11158c7 to 45a239d Compare June 5, 2025 14:51
@HW42 HW42 changed the title RFC: Always set _closed future in StdoutWriterProtocol.connection_lost Always set _closed future in StdoutWriterProtocol.connection_lost Jun 5, 2025
@HW42 HW42 marked this pull request as ready for review June 5, 2025 16:30
This was referenced Jun 5, 2025
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
openqa-pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HW42 @marmarek @qubesos-bot