Skip to content

Fixed race condition on Named pipe dispose/disconnect - #571

Merged
rosenbjerg merged 15 commits into
rosenbjerg:mainfrom
techtel-pstevens:main
Oct 17, 2025
Merged

rosenbjerg merged 15 commits into
rosenbjerg:mainfrom
techtel-pstevens:main

Conversation

@techtel-pstevens

Copy link
Copy Markdown
Contributor

No description provided.

@techtel-pstevens

Copy link
Copy Markdown
Contributor Author

fixes this issue:

#570

@techtel-pstevens

techtel-pstevens commented Jul 7, 2025 •

Copy link
Copy Markdown
Contributor Author

this is a quick fix minimizing the impact of changes to one class.

@rosenbjerg
rosenbjerg requested a review from Copilot October 16, 2025 11:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR aims to fix a race condition around disposing and disconnecting a NamedPipeServerStream in PipeArgument by introducing locking around those operations. It also rearranges TestContext properties in tests and slightly reformats one test call for readability.

  • Add locking around Pipe dispose (Post) and disconnect (During finally)
  • Move TestContext to the top of test classes and reformat a long call argument

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Adds lock statements around Pipe.Dispose() and Pipe.Disconnect() to prevent races
FFMpegCore.Test/VideoTest.cs Moves TestContext property to top of class (no functional change)
FFMpegCore.Test/FFProbeTests.cs Moves TestContext to top and wraps long call across lines (no functional change)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Outdated
Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Outdated
Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Outdated
Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Outdated
@rosenbjerg
rosenbjerg requested a review from Copilot October 16, 2025 11:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Outdated
Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs Outdated
Comment thread FFMpegCore/FFMpeg/Arguments/PipeArgument.cs
@codecov-commenter

codecov-commenter commented Oct 16, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.58%. Comparing base (1c4333e) to head (767a3f5).
⚠️ Report is 113 commits behind head on main.

Files with missing lines Patch % Lines
FFMpegCore/FFMpeg/Arguments/PipeArgument.cs 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #571      +/-   ##
==========================================
- Coverage   71.20%   68.58%   -2.62%     
==========================================
  Files         117      122       +5     
  Lines        2973     3467     +494     
  Branches      284      305      +21     
==========================================
+ Hits         2117     2378     +261     
- Misses        751      974     +223     
- Partials      105      115      +10     

☔ 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.

@rosenbjerg

Copy link
Copy Markdown
Owner

Hey @techtel-pstevens,
Finally spent the time to fix CI and also made some minor improvements
Thanks for the contribution!

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.

4 participants