Skip to content

Conversation

@cce
Copy link
Contributor

@cce cce commented Jun 28, 2024

Summary

Mentioned in #6042, the network interface has since #1390 required a method GetHTTPRequestConnection so that an HTTP handler could call SetWriteDeadline to set a request-specific write deadline. (It is used for setting a longer write deadline when serving large catchpoint snapshot files.)

In Go 1.20 per-request deadlines were added to the builtin HTTP library using http.ResponseController, which makes it very easy to do from inside a HTTP handler. This makes the connection-tracking middleware in #1390 and #5924 to implement GossipNode.GetHTTPRequestConnection obsolete.

For reviewers: the deleted lines all come from #1390 and #5924, so if you open up those side-by-side you should be able to see what is being removed vs. what is being left.

Test Plan

Existing tests should pass, and maybe a new test should be added to ledgerService_test.go to ensure the write deadlines are working as described when using (http.ResponseController).SetWriteDeadline.

@codecov
Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.13%. Comparing base (c8407ab) to head (d8c2922).

Files Patch % Lines
rpcs/ledgerService.go 57.14% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6044      +/-   ##
==========================================
- Coverage   56.15%   56.13%   -0.03%     
==========================================
  Files         488      488              
  Lines       69532    69425     -107     
==========================================
- Hits        39044    38969      -75     
+ Misses      27832    27804      -28     
+ Partials     2656     2652       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cce cce changed the base branch from feature/p2p to master June 28, 2024 18:27
@cce cce force-pushed the http-responsecontroller branch from b042bc0 to d8c2922 Compare June 28, 2024 18:33
@cce cce marked this pull request as ready for review July 1, 2024 14:45
@cce cce added the Enhancement label Jul 1, 2024
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Nice work

@algorandskiy algorandskiy requested a review from gmalouf July 1, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants