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 two bugs from metrics and cancel #399

Merged
merged 11 commits into from
Dec 22, 2023
Merged

fix two bugs from metrics and cancel #399

merged 11 commits into from
Dec 22, 2023

Conversation

TingDaoK
Copy link
Contributor

Issue #, if available:

  • The downstream CI caught the bugs.
  • One is when we have pending streaming requests in the list, we finishing the meta request by just release the refcount on those requests. But we wait the eventloop to delivery the metrics while they delivery the body streaming for those requests. So, those requests will leak the metrics memory
  • The second one is that when the cancel for the http stream was invoked after the stream has already completed, or completing. The error code on complete will not be the error code we asked for canceling. So, we cannot rely on the error code to check for the node has been removed or not.

Description of changes:

  • Finish up the metrics from the meta request finish function as well. Double-checked that nothing else will pop from the priority list to deliver the body
  • Rely on the synced_data.http_stream to track the linked-list node should be removed or not

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@TingDaoK TingDaoK marked this pull request as ready for review December 21, 2023 01:01
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (f1ba15e) 88.74% compared to head (33e40a3) 88.97%.

❗ Current head 33e40a3 differs from pull request most recent head e10dbd2. Consider uploading reports for the commit e10dbd2 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   88.74%   88.97%   +0.23%     
==========================================
  Files          21       21              
  Lines        6024     6043      +19     
==========================================
+ Hits         5346     5377      +31     
+ Misses        678      666      -12     
Files Coverage Δ
source/s3_auto_ranged_put.c 92.58% <100.00%> (ø)
source/s3_util.c 99.63% <100.00%> (+<0.01%) ⬆️
source/s3_meta_request.c 92.08% <88.63%> (+1.46%) ⬆️

@TingDaoK TingDaoK merged commit 20097d3 into main Dec 22, 2023
30 checks passed
@TingDaoK TingDaoK deleted the fix-metrics branch December 22, 2023 21:01
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.

3 participants