Skip to content

[Windows] Check for errors when connecting with client#182

Merged
wllenyj merged 1 commit intocontainerd:masterfrom
jsturtevant:fix-get-client-connection
Apr 29, 2023
Merged

[Windows] Check for errors when connecting with client#182
wllenyj merged 1 commit intocontainerd:masterfrom
jsturtevant:fix-get-client-connection

Conversation

@jsturtevant
Copy link
Copy Markdown
Collaborator

The unwrap() on the file when creating the client connection causes a panic. This can happen if the file doesn't exist or there is a small chance when multiple clients connect quickly that the pipe returns 'All pipe instances are busy.' also causing a panic. This allows the caller to handle these errors and retry if necessary (as is the case with the pipe instances being busy).

@jsturtevant jsturtevant changed the title Check for errors when connecting to client [Windows] Check for errors when connecting to client Apr 11, 2023
@jsturtevant jsturtevant changed the title [Windows] Check for errors when connecting to client [Windows] Check for errors when connecting with client Apr 11, 2023
@jsturtevant jsturtevant force-pushed the fix-get-client-connection branch from d4a795b to 26c9c2f Compare April 12, 2023 00:01
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (6639bca) 24.39% compared to head (d96bea3) 24.39%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #182   +/-   ##
=======================================
  Coverage   24.39%   24.39%           
=======================================
  Files          17       17           
  Lines        2529     2529           
=======================================
  Hits          617      617           
  Misses       1912     1912           
Impacted Files Coverage Δ
src/error.rs 55.55% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

The unwrap() causes a panic.  This can happen if the file doesn't exist or there is a small chance when mutliple clients connect quickly that the pipe returns 'All pipe instances are busy.' also causing a panic.  This allows the caller to handle these errors and retry if necessary (as is the case with the pipe instances being busy).

Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant jsturtevant force-pushed the fix-get-client-connection branch from 26c9c2f to d96bea3 Compare April 12, 2023 00:02
@jsturtevant jsturtevant requested a review from lifupan April 12, 2023 03:48
Copy link
Copy Markdown
Member

@Tim-Zhang Tim-Zhang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jsturtevant

Copy link
Copy Markdown
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Collaborator

@quanweiZhou quanweiZhou left a comment

Choose a reason for hiding this comment

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

Thanks @jsturtevant, LGTM.

@jsturtevant
Copy link
Copy Markdown
Collaborator Author

@wllenyj @Tim-Zhang friendly ping for a merge.

I think these last couple PR should have stabilized the windows support. If we can get these merged and a release out then I start using this in containerd/rust-extensions#139. If there is any other work before a release please let me know and I can try to help out.

@wllenyj wllenyj merged commit 2396cf3 into containerd:master Apr 29, 2023
KarstenB pushed a commit to KarstenB/ttrpc-rust that referenced this pull request May 1, 2025
…nection

[Windows] Check for errors when connecting with client
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.

5 participants