Skip to content
New issue

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

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

Already on GitHub? # to your account

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

Merged
merged 1 commit into from
Apr 29, 2023

Conversation

jsturtevant
Copy link
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
@codecov
Copy link

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>
Copy link
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
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
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
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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants