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

feat(routing/http/server): add routing timeout #720

Merged
merged 3 commits into from
Nov 19, 2024
Merged

feat(routing/http/server): add routing timeout #720

merged 3 commits into from
Nov 19, 2024

Conversation

2color
Copy link
Member

@2color 2color commented Nov 14, 2024

Why

See ipfs/someguy#88.

TL;DR: Without any routing timeout, routing requests will run indefinitely if the record limit isn't reached.

@2color 2color requested a review from a team as a code owner November 14, 2024 10:52
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 60.35%. Comparing base (1bf1488) to head (d699f73).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
routing/http/server/server.go 80.95% 4 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
+ Coverage   60.34%   60.35%   +0.01%     
==========================================
  Files         243      243              
  Lines       31039    31056      +17     
==========================================
+ Hits        18730    18745      +15     
- Misses      10644    10645       +1     
- Partials     1665     1666       +1     
Files with missing lines Coverage Δ
routing/http/server/server.go 74.30% <80.95%> (+0.08%) ⬆️

... and 5 files with indirect coverage changes


🚨 Try these New Features:

@lidel lidel self-assigned this Nov 18, 2024
@lidel lidel mentioned this pull request Nov 18, 2024
47 tasks
@lidel lidel changed the title feat: add routing timeouts to delegated server feat(routing/http/server): add routing timeout Nov 19, 2024
@lidel lidel changed the base branch from add-instrumentation to main November 19, 2024 00:10
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Tested in ipfs/someguy#87, lgtm, thank you!

CHANGELOG.md Outdated Show resolved Hide resolved
@lidel lidel merged commit e38f236 into main Nov 19, 2024
15 checks passed
@lidel lidel deleted the add-timeouts branch November 19, 2024 00:31
lidel added a commit to ipfs/someguy that referenced this pull request Nov 19, 2024
2color added a commit to ipfs/someguy that referenced this pull request Nov 19, 2024
…request timeouts (#87)

* fix: larger duration buckets for better visibility

* feat: log accept header

* fix: move instrumentation to boxo

* feat: add tracing with auth token

* feat: add 30 second request timeout

* chore: remove replace directive

* chore: add missing funcSampler

* chore: remove request timeout

this isn't working too well. We need to look more deeply into this

* chore: update changelog

* chore: go mod tidy

* chore: go-libp2p-kad-dht v0.28.1

* chore: latest boxo#720

* chore: mod tidy

* chore: boxo main

with ipfs/boxo#720 and ipfs/boxo#718

* Apply suggestions from code review

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* fix: typo

---------

Co-authored-by: Daniel N <2color@users.noreply.github.com>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
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.

2 participants