Time-out starknet_getTransactionStatus after 5s #1619
Conversation
joshklop
force-pushed
the
joshklop/cancel-gettxstatus
branch
from
1a52dfb to
62f48d7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1619 +/- ##
==========================================
+ Coverage 78.37% 78.42% +0.05%
==========================================
Files 100 100
Lines 9221 9224 +3
==========================================
+ Hits 7227 7234 +7
+ Misses 1355 1352 -3
+ Partials 639 638 -1 ☔ View full report in Codecov by Sentry. |
rpc/handlers.go
Outdated
| const timeout = 5 * time.Second | ||
| ctxChild, cancelChild := context.WithTimeout(ctx, timeout) | ||
| txStatus, err := h.feederClient.Transaction(ctxChild, &hash) | ||
| cancelChild() |
There was a problem hiding this comment.
Instead of this, should we make feeder.Client respect the timeout value that we pass to it by WithTimeout regardless of how many times it retries to complete the request?
There was a problem hiding this comment.
Maybe, but I think the current method of setting a per-request timeout and then controlling the overall timeout with a context also makes sense (makes me wonder if a per-request timeout is even necessary, but that's out-of-scope for this PR).
There was a problem hiding this comment.
controlling the overall timeout with a context
That didn't work for Gateway client tho, that is why we ended up having to do #1614. I am worried that we will end up having to do the same here as well.
to avoid sending 429s after waiting for 30s, due to feeder rate limiting.
kirugan
force-pushed
the
joshklop/cancel-gettxstatus
branch
from
62f48d7 to
fb814c7
Compare
|
Decided to close the PR. We already have timeout setting for feeder client which is larger than 5 sec. |
to avoid sending 429s after waiting for 30s, due to feeder rate limiting.