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

fix shutdown hang when enable socket #835

Merged
merged 3 commits into from
Nov 29, 2021
Merged

fix shutdown hang when enable socket #835

merged 3 commits into from
Nov 29, 2021

Conversation

daixiang0
Copy link
Member

@daixiang0 daixiang0 commented Nov 16, 2021

Signed-off-by: Loong loong.dai@intel.com

Description

Go-retryablehttp cannot close idle socket connections, need to call it by hand.

Fix dapr/dapr#3894

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@daixiang0 daixiang0 requested review from a team as code owners November 16, 2021 05:59
@daixiang0
Copy link
Member Author

daixiang0 commented Nov 16, 2021

@mukundansundar please review, also it works when directly use native HTTP client.

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #835 (a8cf37c) into master (20c3640) will increase coverage by 0.54%.
The diff coverage is 79.41%.

❗ Current head a8cf37c differs from pull request most recent head bbc8260. Consider uploading reports for the commit bbc8260 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   21.82%   22.36%   +0.54%     
==========================================
  Files          29       29              
  Lines        1535     1547      +12     
==========================================
+ Hits          335      346      +11     
- Misses       1159     1160       +1     
  Partials       41       41              
Impacted Files Coverage Δ
pkg/metadata/metadata.go 2.08% <0.00%> (-0.10%) ⬇️
pkg/standalone/testutils.go 75.86% <76.19%> (+3.44%) ⬆️
pkg/standalone/publish.go 90.24% <100.00%> (+3.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20c3640...bbc8260. Read the comment docs.

Signed-off-by: Loong <loong.dai@intel.com>
@mukundansundar
Copy link
Collaborator

mukundansundar commented Nov 18, 2021

@daixiang0 Please change this line to add back socket test cases
https://github.com/dapr/cli/pull/835/files#diff-40b8b5acdd5260df8de205a924b446eb2483901d4e4c30c6a23932d3223e99e9L42

Line 42 in standalone_test.go file.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Line 42 in standalone_test.go should now be:

var socketCases = []string{"", "/tmp"}

@daixiang0
Copy link
Member Author

@mukundansundar @berndverst done.

Signed-off-by: Loong <loong.dai@intel.com>
@daixiang0
Copy link
Member Author

@dapr/maintainers-cli please review.

Copy link
Member

@yaron2 yaron2 left a comment

Choose a reason for hiding this comment

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

lgtm

@yaron2 yaron2 merged commit 6b13457 into dapr:master Nov 29, 2021
berndverst pushed a commit to berndverst/cli that referenced this pull request Nov 29, 2021
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
artursouza pushed a commit that referenced this pull request Nov 29, 2021
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>

Co-authored-by: Long Dai <long.dai@intel.com>
Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
@daixiang0 daixiang0 deleted the hang branch November 30, 2021 01:10
anhldbk pushed a commit to anhldbk/dapr-cli that referenced this pull request Dec 22, 2021
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
anhldbk pushed a commit to anhldbk/dapr-cli that referenced this pull request Dec 22, 2021
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
anhldbk pushed a commit to anhldbk/dapr-cli that referenced this pull request Dec 22, 2021
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
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.

Fix unix domain socket shutdown call
4 participants