refactor: HTTP config #2278
refactor: HTTP config #2278
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2278 +/- ##
===========================================
- Coverage 74.03% 73.97% -0.07%
===========================================
Files 256 256
Lines 25695 25587 -108
===========================================
- Hits 19023 18926 -97
+ Misses 5356 5346 -10
+ Partials 1316 1315 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
http/server.go
Outdated
| } | ||
| // ignore close errors as they cannot be handled | ||
| // from the caller of this method | ||
| defer listener.Close() //nolint:errcheck |
There was a problem hiding this comment.
question: Why can they not be handled but other errors appear to be so? And is that not up to any callers of this public function to decide? Should the choice to discard the error not be made by the caller?
There was a problem hiding this comment.
I've removed this Close call as it is redundant. After taking a closer look the Serve and ServeTLS functions will close the listener.
http/server.go
Outdated
| return nil | ||
| // Server struct holds the Handler for the HTTP API. | ||
| type Server struct { | ||
| address atomic.Value |
There was a problem hiding this comment.
question: Why is this atomic? It only appears to be set once, in ListenAndServe, and that function cannot be called multiple times concurrently no?
todo: If it remains atomic, please document why.
There was a problem hiding this comment.
I've added documentation to clarify the use of the atomic value.
There was a problem hiding this comment.
Ah, you want AssignedAddr to return empty if ListenAndServe is in progress? Would it not be better to lock, and block AssignedAddr from returning until the address has been assigned? (i.e. no atomic, but a RWLock opened on the first line of ListenAndServe)
There was a problem hiding this comment.
I agree that would be better, but I realized we don't use this method anywhere outside of server_test.go. I've removed the AssignedAddr function and address property from server.
As a side note: tests that need a random port assignment should use the httptest package instead.
There was a problem hiding this comment.
As a side note: tests that need a random port assignment should use the httptest package instead.
That's a really nice change. Thanks for removing the need to keep track of the randomly assigned port on the http server :)
## Relevant issue(s) Resolves sourcenetwork#2277 ## Description This PR is a refactor of the HTTP config and is part of the larger config refactor sourcenetwork#2257 ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? make test Specify the platform(s) on which this was tested: - MacOS
Relevant issue(s)
Resolves #2277
Description
This PR is a refactor of the HTTP config and is part of the larger config refactor #2257
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: