ipfs: Remove concurrency limit, rely on rate limit #4570
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The concurrency limit, because it uses a semaphore, had unfortunate interactions with the use of `call_all` in the polling monitor. `CallAll` can internally have requests which are holding the semaphore, so it must be polled regularly to check on those requests. If the task holding the `CallAll` hangs on some other future, that can lead to a deadlock. In our case, it can hang in the `response_sender.send((id, response)).await`, if the channel is full and the subgraph runner takes a long time to check it. This could be fixed, but the footgun would remain so it seemed best to rely only on the rate limit which can't deadlock.
lutter
approved these changes
Apr 25, 2023
| - `GRAPH_IPFS_REQUEST_LIMIT`: Limits both concurrent and per second requests to IPFS for file data | ||
| sources. Defaults to 100. | ||
| - `GRAPH_IPFS_REQUEST_LIMIT`: Limits both per second requests to IPFS for file data sources. | ||
| Defaults to 100. |
There was a problem hiding this comment.
Maybe better: 'Limits the number of requests per second to IPFS for ...'
leoyvens
added a commit
that referenced
this pull request
May 26, 2023
This addresses both sides of the issue, by making sure the task holding the `CallAll` doesn't hang, and by removing the concurrency control done by `Buffer`, which may be the reason why PR #4570 didn't fully work.
leoyvens
added a commit
that referenced
this pull request
May 26, 2023
This addresses both sides of the issue, by making sure the task holding the `CallAll` doesn't hang, and by removing the concurrency control done by `Buffer`, which may be the reason why PR #4570 didn't fully work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The concurrency limit, because it uses a semaphore, had unfortunate interactions with the use of
call_allin the polling monitor.CallAllcan internally have requests which are holding the semaphore, so it must be polled regularly to check on those requests. If the task holding theCallAllhangs on some other future, that can lead to a deadlock.In our case, it can hang in the
response_sender.send((id, response)).await, if the channel is full and the subgraph runner takes a long time to check it:graph-node/core/src/polling_monitor/mod.rs
Line 155 in 9f4a182
This could be fixed, but the footgun would remain so it seemed best to rely only on the rate limit which can't deadlock.
This also adds a log when a file is not found.