Skip to content

Fix healthz socket #2737

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
Merged
merged 3 commits into from
Feb 18, 2021
Merged

Fix healthz socket #2737

merged 3 commits into from
Feb 18, 2021

Conversation

@code-asher
Copy link
Member

@code-asher code-asher commented Feb 16, 2021 •
edited
Loading

Was having problems with the healthz socket, turns out I forgot to resume. Added some tests while I was at it.

@code-asher code-asher changed the base branch from v3.9.0 to master February 16, 2021 20:16
@code-asher code-asher requested a review from jsjoeio February 16, 2021 20:18
@code-asher
Force shutdown sockets during tests
59ba78c
@code-asher
Add healthz tests
7f80d15
@code-asher
Fix health socket not getting client messages
2d8b785 
Forgot to resume. Went ahead and did the same for the test plugin
although it only sends messages and doesn't receive any.
jsjoeio
jsjoeio approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Woo! Nice work and thanks for adding some tests 🎉

test/httpserver.ts
Comment on lines +76 to +77
// If there are sockets remaining we might need to force close them or
// this promise might never resolve.
Copy link
Contributor

@jsjoeio jsjoeio Feb 18, 2021

Choose a reason for hiding this comment

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

Great comments! 👏

test/httpserver.ts
// If there are sockets remaining we might need to force close them or
// this promise might never resolve.
if (this.sockets.size > 0) {
// Give sockets a chance to close up shop.
Copy link
Contributor

@jsjoeio jsjoeio Feb 18, 2021

Choose a reason for hiding this comment

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

😎

@jsjoeio
Copy link
Contributor

jsjoeio commented Feb 18, 2021

CI failure looks like a timeout from something external.

##### Begin Diagnostic HTTP information #####
Status Code: 404
Status Message: Not Found
Header Information: {
  "cache-control": "no-store,no-cache",
  "pragma": "no-cache",
  "content-length": "348",
  "content-type": "application/json; charset=utf-8",
  "strict-transport-security": "max-age=2592000",
  "x-tfs-processid": "7e603661-ddb4-43d1-9f0e-8e9afb096e04",
  "activityid": "1e551a6d-73b2-4a13-a645-9c3b05f1c5da",
  "x-tfs-session": "1e551a6d-73b2-4a13-a645-9c3b05f1c5da",
  "x-vss-e2eid": "1e551a6d-73b2-4a13-a645-9c3b05f1c5da",
  "x-vss-senderdeploymentid": "193695a0-0dcd-ade4-f810-b10ad24a9829",
  "x-frame-options": "SAMEORIGIN",
  "x-msedge-ref": "Ref A: 1350B25CB9A64B848CF227FD2CC0A533 Ref B: BLUEDGE0812 Ref C: 2021-02-16T21:30:02Z",
  "date": "Tue, 16 Feb 2021 21:30:01 GMT"
}
###### End Diagnostic HTTP information ######
Error: Unable to download the artifact: Error: Unexpected http 404 during download for https://pipelines.actions.githubusercontent.com/5X3JLcJ0lQdKyG57CykbBROv4yhW2GlZguFB0LFiM9s362wqzL/_apis/resources/Containers/9411325?itemPath=release-packages%2Fcode-server_3.9.0_amd64.deb

@code-asher code-asher merged commit 02f9af1 into master Feb 18, 2021
@code-asher code-asher deleted the health-test branch February 18, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jsjoeio jsjoeio jsjoeio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@code-asher @jsjoeio