Skip to content
This repository has been archived by the owner on Oct 25, 2024. It is now read-only.

chore: fuel-indexer: refactor run, service, and executor to take advantage of tokio::task::JoinSet #1327

Merged
merged 11 commits into from
Sep 12, 2023

Conversation

lostman
Copy link
Contributor

@lostman lostman commented Sep 4, 2023

Description

This PR cleans up parts of IndexerService, Executor, and fuel-indexer's run.rs.

There is a slight change of semantics due to the replacement of FuturesUnordered with JoinSet: JoinSet aborts all tasks when it is dropped. This appeared in web-api tests that dropped the IndexerService object before the tests' completion. Tasks added to FuturesUnordered would've been running anyway (since task::spawn detaches the future). Tasks spawned into the task set of IndexerService are aborted. This required a small change to the tests to keep the IndexerService alive.

Testing steps

CI tests should pass.

Changelog

  • Replace FuturesUnordered with tokio::task::JoinSet
  • Clean up code in service.rs and executor.rs: move the kill switch into Executor implementations and expose through the trait interface; expose the manifest the same way. This allows cleaning up some functions that now only need the executor and no other parameters.
  • Simplify fuel-indexer's run.rs to use the JoinSet for all subsystems (node, web-api, signal-handler, etc.).
  • WasmIndexExecutor::new() takes WASM bytes and not ExecSource: we were passing ExecSource::Manifest and returning ExecSource::Registry(wasm_bytes). Instead, the WASM bytes are loaded before new() is called, and everything is simpler this way.
  • IndexerService::run removes completed tasks from the JoinSet and runs the service loop: loop { tokio::select! ... }. This way, we don't need two separate tasks for it.

Measuring resource usage:

Changes in this PR have no adverse effect on the resource usage:

Screenshot 2023-09-08 at 12 55 33 PM

@lostman lostman self-assigned this Sep 4, 2023
@lostman lostman force-pushed the maciej/joinset branch 2 times, most recently from 79b48bc to b0d8e1d Compare September 4, 2023 14:18
@lostman lostman marked this pull request as ready for review September 5, 2023 10:02
Copy link
Contributor

@deekerno deekerno left a comment

Choose a reason for hiding this comment

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

This looks pretty good! I like the cleanup. I left a few comments to be addressed before approval.

packages/fuel-indexer/src/commands/run.rs Outdated Show resolved Hide resolved
packages/fuel-indexer/src/commands/run.rs Outdated Show resolved Hide resolved
packages/fuel-indexer/src/commands/run.rs Outdated Show resolved Hide resolved
packages/fuel-indexer/src/executor.rs Outdated Show resolved Hide resolved
packages/fuel-indexer/src/service.rs Outdated Show resolved Hide resolved
@lostman lostman requested a review from deekerno September 12, 2023 12:15
Copy link
Contributor

@deekerno deekerno left a comment

Choose a reason for hiding this comment

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

🆗

@lostman lostman merged commit 651c9ce into develop Sep 12, 2023
18 checks passed
@lostman lostman deleted the maciej/joinset branch September 12, 2023 13:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants