experimenting: seeding worker #218
Conversation
hlolli
commented
Oct 4, 2024
hlolli
commented
- manage it with env-var based filters
- add webtorrent dependency
- implement method seed to SeedingWorker class
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve updates to several files, primarily focusing on enhancing the environment variable configuration, adding new dependencies, and introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant System
participant SeedingWorker
participant ContiguousDataSource
System->>System: on(events.TX_INDEXED)
alt If seedingWorkerFilter is defined and tx.id is available
System->>SeedingWorker: seed(tx.id)
SeedingWorker->>ContiguousDataSource: retrieve data stream for tx.id
SeedingWorker->>SeedingWorker: log start of seeding process
SeedingWorker->>SeedingWorker: use WebTorrent to seed data
SeedingWorker->>SeedingWorker: log magnet URI
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #218 +/- ##
===========================================
- Coverage 68.72% 68.68% -0.04%
===========================================
Files 32 32
Lines 7919 7941 +22
Branches 431 433 +2
===========================================
+ Hits 5442 5454 +12
- Misses 2476 2486 +10
Partials 1 1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/workers/seeding-worker.ts (1)
23-39: LGTM: Class declaration and constructor are well-structured.The SeedingWorker class is well-defined with appropriate private and public properties. The constructor correctly initializes the WebTorrent client and sets up a child logger.
Consider adding a type annotation for the
webTorrentClientproperty:public webTorrentClient: WebTorrent.Instance;This will improve type safety and code readability.
docs/envs.md (2)
29-29: Approve new SEEDING_WORKER_FILTER variable with a suggestion for improvement.The addition of the SEEDING_WORKER_FILTER environment variable aligns well with the PR objectives for managing the seeding worker. The default value of 'undefined' is a safe choice.
Consider expanding the description to provide more context or examples of valid filter values. This would help users understand how to effectively use this setting.
30-30: Approve ARNS_ROOT_HOST update with a suggestion for documentation.The update to ARNS_ROOT_HOST, setting the default value to 'undefined', makes this setting optional. This change provides more flexibility but may require user attention.
Consider adding a note in the description or elsewhere in the documentation to explain the implications of leaving this value undefined and when users should configure it.
src/system.ts (3)
244-253: LGTM: SeedingWorker integration looks good, consider adding error handling.The integration of the
SeedingWorkerwithin theTX_INDEXEDevent listener is well-implemented. It correctly checks for the existence ofseedingWorkerFilterandtx.idbefore applying the filter and triggering the seeding operation.Consider adding error handling for the
seedingWorker.seed(tx.id)call to ensure any errors during seeding are properly logged and don't disrupt the event processing flow. For example:if ( seedingWorkerFilter !== undefined && tx.id !== undefined && (await seedingWorkerFilter.match(tx)) ) { - seedingWorker.seed(tx.id); + try { + await seedingWorker.seed(tx.id); + } catch (error) { + log.error('Error seeding transaction', { txId: tx.id, error }); + } }
630-630: LGTM: SeedingWorker instantiation is correct, consider adding configuration options.The
SeedingWorkeris properly instantiated with the necessarylogandcontiguousDataSourcedependencies, and it's correctly exported for use in other modules.Consider allowing for more configuration options to be passed to the
SeedingWorker. This could provide more flexibility in how the seeding process operates. For example:- export const seedingWorker = new SeedingWorker({ log, contiguousDataSource }); + export const seedingWorker = new SeedingWorker({ + log, + contiguousDataSource, + maxConcurrentSeeds: config.MAX_CONCURRENT_SEEDS, + seedingTimeout: config.SEEDING_TIMEOUT, + });This assumes you have or will add these configuration options to your config file. Adjust the specific options based on what parameters the
SeedingWorkerconstructor actually accepts.
Line range hint
633-678: Consider adding SeedingWorker to the shutdown process.The
shutdownfunction doesn't include any cleanup for the newly addedSeedingWorker. To ensure a graceful shutdown of all components, consider adding theSeedingWorkerto this process.Add the
SeedingWorkerto the shutdown sequence:export const shutdown = async (express: Server) => { if (isShuttingDown) { log.info('Shutdown already in progress'); } else { isShuttingDown = true; log.info('Shutting down...'); express.close(async () => { log.debug('Web server stopped successfully'); eventEmitter.removeAllListeners(); arIODataSource.stopUpdatingPeers(); dataSqliteWalCleanupWorker?.stop(); parquetExporter?.stop(); await arnsResolverCache.close(); await mempoolWatcher?.stop(); await blockImporter.stop(); await dataItemIndexer.stop(); await txRepairWorker.stop(); await txImporter.stop(); await txFetcher.stop(); await txOffsetImporter.stop(); await txOffsetRepairWorker.stop(); await bundleDataImporter.stop(); await bundleRepairWorker.stop(); await ans104DataIndexer.stop(); await ans104Unbundler.stop(); await webhookEmitter.stop(); + await seedingWorker.stop(); await db.stop(); await headerFsCacheCleanupWorker?.stop(); await contiguousDataFsCacheCleanupWorker?.stop(); process.exit(0); }); } };This assumes that the
SeedingWorkerclass has astop()method. If it doesn't, you may need to implement one to properly clean up any resources or ongoing operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
- docs/envs.md (1 hunks)
- package.json (2 hunks)
- src/config.ts (1 hunks)
- src/system.ts (3 hunks)
- src/workers/seeding-worker.ts (1 hunks)
🔇 Additional comments (11)
src/workers/seeding-worker.ts (3)
1-17: LGTM: License header is present and correct.The GNU Affero General Public License header is properly included at the beginning of the file, which is appropriate for open-source software.
19-21: LGTM: Imports are appropriate and align with PR objectives.The imports include necessary dependencies for logging (winston), WebTorrent functionality, and the ContiguousDataSource type. These align well with the class's requirements and the PR's objectives.
1-61: Overall, the SeedingWorker implementation looks good with room for minor improvements.The SeedingWorker class successfully implements the seeding functionality as described in the PR objectives. It properly integrates the WebTorrent library and provides a structured approach to manage the seeding of data streams.
Key points:
- The license header is correctly included.
- Imports are appropriate and align with the PR objectives.
- The class structure is well-defined with proper use of private and public modifiers.
- The seeding functionality is implemented in the
seedmethod.The suggested improvements, including adding a type annotation for
webTorrentClientand refactoring theseedmethod for better Promise handling, will further enhance the code quality, type safety, and error handling.package.json (3)
51-51: Summary: Changes align well with PR objectives.The additions of
webtorrentand@types/webtorrentare focused and directly support the PR's goal of initializing a seeding worker. These changes are minimal and don't introduce any apparent conflicts with existing dependencies.As a final check, please ensure that the project builds and runs correctly with these new dependencies.
Also applies to: 74-74
74-74: LGTM: @types/webtorrent devDependency added.The addition of
"@types/webtorrent": "^0.109.8"is appropriate for TypeScript support. This aligns well with the project's use of TypeScript.To ensure compatibility between webtorrent and its type definitions, please run:
#!/bin/bash echo "webtorrent version:" npm show webtorrent version echo "@types/webtorrent version:" npm show @types/webtorrent versionCompare the outputs and ensure they are compatible. If there's a mismatch, consider updating to matching versions.
51-51: LGTM: webtorrent dependency added.The addition of
"webtorrent": "^2.5.1"is consistent with the PR's objective of implementing seeding functionality. The caret (^) allows for compatible updates, which is a good practice.To ensure we're using the latest stable version, please run:
Compare the output with the current version "2.5.1" and update if necessary.
src/config.ts (1)
92-100: Approve with suggestions: Add error handling and improve documentationThe implementation of the new seeding worker filter configuration is good, allowing for dynamic configuration through environment variables. However, there are a few suggestions for improvement:
- Add error handling for JSON parsing to prevent potential runtime exceptions:
export const SEEDING_WORKER_FILTER = SEEDING_WORKER_FILTER_STRING === undefined ? undefined - : createFilter(JSON.parse(SEEDING_WORKER_FILTER_STRING)); + : (() => { + try { + return createFilter(JSON.parse(SEEDING_WORKER_FILTER_STRING)); + } catch (error) { + console.error('Error parsing SEEDING_WORKER_FILTER:', error); + return undefined; + } + })();
- Add comments to explain the purpose and usage of these new constants:
+// Environment variable for configuring the seeding worker filter export const SEEDING_WORKER_FILTER_STRING = env.varOrUndefined( 'SEEDING_WORKER_FILTER', ); +// Parsed and created filter for the seeding worker +// If the environment variable is set, it should contain a valid JSON string +// that can be parsed and used to create a filter using the createFilter function export const SEEDING_WORKER_FILTER = SEEDING_WORKER_FILTER_STRING === undefined ? undefined : createFilter(JSON.parse(SEEDING_WORKER_FILTER_STRING));To ensure these new constants are being used correctly in the codebase, please run the following script:
This will help identify where these constants are being used and ensure they are being applied correctly in the seeding worker implementation.
✅ Verification successful
Approve with suggestions: Add error handling and improve documentation
The usage of
SEEDING_WORKER_FILTER_STRINGandSEEDING_WORKER_FILTERhas been verified and is confined tosrc/config.tsandsrc/system.ts, ensuring they are used appropriately within the codebase. To further enhance the implementation:
- Add error handling for JSON parsing to prevent potential runtime exceptions:
export const SEEDING_WORKER_FILTER = SEEDING_WORKER_FILTER_STRING === undefined ? undefined - : createFilter(JSON.parse(SEEDING_WORKER_FILTER_STRING)); + : (() => { + try { + return createFilter(JSON.parse(SEEDING_WORKER_FILTER_STRING)); + } catch (error) { + console.error('Error parsing SEEDING_WORKER_FILTER:', error); + return undefined; + } + })();- Add comments to explain the purpose and usage of these new constants:
+// Environment variable for configuring the seeding worker filter export const SEEDING_WORKER_FILTER_STRING = env.varOrUndefined( 'SEEDING_WORKER_FILTER', ); +// Parsed and created filter for the seeding worker +// If the environment variable is set, it should contain a valid JSON string +// that can be parsed and used to create a filter using the createFilter function export const SEEDING_WORKER_FILTER = SEEDING_WORKER_FILTER_STRING === undefined ? undefined : createFilter(JSON.parse(SEEDING_WORKER_FILTER_STRING));🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of SEEDING_WORKER_FILTER_STRING and SEEDING_WORKER_FILTER # Search for usage of SEEDING_WORKER_FILTER_STRING echo "Usage of SEEDING_WORKER_FILTER_STRING:" rg --type typescript "SEEDING_WORKER_FILTER_STRING" -C 3 echo "\nUsage of SEEDING_WORKER_FILTER:" rg --type typescript "SEEDING_WORKER_FILTER(?!_STRING)" -C 3Length of output: 575
Script:
#!/bin/bash # Description: Verify the usage of SEEDING_WORKER_FILTER_STRING and SEEDING_WORKER_FILTER # Search for usage of SEEDING_WORKER_FILTER_STRING echo "Usage of SEEDING_WORKER_FILTER_STRING:" rg --type ts "SEEDING_WORKER_FILTER_STRING" -C 3 echo "\nUsage of SEEDING_WORKER_FILTER:" rg --type ts "SEEDING_WORKER_FILTER" -C 3 | grep -v "SEEDING_WORKER_FILTER_STRING"Length of output: 1388
docs/envs.md (2)
31-31: Approve SANDBOX_PROTOCOL update with commendation for improved description.The update to SANDBOX_PROTOCOL, setting the default value to 'undefined' and providing a more detailed description, is a positive change. The description now clearly explains the dependency on ARNS_ROOT_HOST, which is crucial information for users.
Great job on improving the description! This change, along with the ARNS_ROOT_HOST update, provides better clarity on ArNS-related configurations.
29-31: Overall assessment: Documentation updates align well with PR objectives.The changes to the environment variables documentation in
docs/envs.mdare consistent with the PR's goal of enhancing seeding worker management and improving ArNS-related configurations. The updates provide:
- A new SEEDING_WORKER_FILTER variable for managing the seeding worker.
- More flexible configurations for ARNS_ROOT_HOST and SANDBOX_PROTOCOL.
- Improved descriptions that offer better guidance to users.
These changes contribute to a more configurable and user-friendly ar.io node setup.
Great job on updating the documentation to reflect the new features and configuration options!
src/system.ts (2)
83-83: LGTM: SeedingWorker import added correctly.The import statement for
SeedingWorkeris properly placed alongside other worker imports and follows the correct relative path structure.
Line range hint
1-678: Overall, the SeedingWorker integration is well-implemented with room for minor enhancements.The changes to integrate the
SeedingWorkerare generally well-done and align with the PR objectives. The code follows existing patterns in the file and is placed appropriately within the system structure.Here's a summary of the suggestions for improvement:
- Add error handling to the seeding operation in the event listener.
- Consider adding more configuration options to the
SeedingWorkerinstantiation for increased flexibility.- Include the
SeedingWorkerin the shutdown process to ensure proper cleanup.These enhancements will improve the robustness and configurability of the new seeding feature. Overall, good job on the implementation!
src/workers/seeding-worker.ts
Outdated
| async seed(txId: string) { | ||
| this.log.debug(`Seeding ${txId}`); | ||
| const data = await this.contiguousDataSource.getData({ id: txId }); | ||
| new Promise<void>((resolve) => | ||
| this.webTorrentClient.seed( | ||
| data.stream, | ||
| { | ||
| announce: [ | ||
| 'wss://tracker.btorrent.xyz', | ||
| 'wss://tracker.openwebtorrent.com', | ||
| 'wss://tracker.webtorrent.io', | ||
| ], | ||
| }, | ||
| (torrent: WebTorrent.Torrent) => { | ||
| this.log.debug(`Seeding ${txId} started: ${torrent.magnetURI}`); | ||
| resolve(); | ||
| }, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Improve Promise handling in the seed method.
The reviewer's concern about Promise handling in the seed method is valid. The webTorrentClient.seed function is only used within src/workers/seeding-worker.ts, specifically in the seed method, and the current implementation does not properly handle the returned Promise.
Recommendations:
- Return the Promise: Ensure that the
seedmethod returns the Promise to allow proper asynchronous handling. - Error Handling: Add error handling by rejecting the Promise on failure to manage potential errors gracefully.
Implementing these changes will enhance the reliability and maintainability of the seeding functionality.
🔗 Analysis chain
Improve Promise handling in the seed method.
The seed method correctly implements the seeding functionality as described in the PR objectives. However, there are some improvements that can be made to the Promise handling:
- The method is declared as async but doesn't use await or return the Promise.
- The Promise creation is unnecessary since WebTorrent's seed method already returns a Promise.
Consider refactoring the method as follows:
async seed(txId: string): Promise<void> {
this.log.debug(`Seeding ${txId}`);
const data = await this.contiguousDataSource.getData({ id: txId });
return new Promise<void>((resolve, reject) => {
this.webTorrentClient.seed(
data.stream,
{
announce: [
'wss://tracker.btorrent.xyz',
'wss://tracker.openwebtorrent.com',
'wss://tracker.webtorrent.io',
],
},
(torrent: WebTorrent.Torrent) => {
this.log.debug(`Seeding ${txId} started: ${torrent.magnetURI}`);
resolve();
}
).on('error', reject);
});
}This refactored version:
- Properly returns the Promise.
- Adds error handling with the
rejectcallback. - Uses a more idiomatic Promise-based approach.
To ensure that the seed method is the only place where seeding is initiated, run the following command:
This will help verify that we're not missing any other locations where seeding might be happening.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type typescript 'webTorrentClient\.seed'
Length of output: 82
Script:
#!/bin/bash
# Searching for 'webTorrentClient.seed' in TypeScript files
rg 'webTorrentClient\.seed' --glob "*.ts"
Length of output: 107
hlolli
force-pushed
the
feature/PE-6874_webtorrent
branch
from
2fa5e62 to
45f8225
Compare
- manage it with env-var based filters - add webtorrent dependency - implement method seed to SeedingWorker class
hlolli
force-pushed
the
feature/PE-6874_webtorrent
branch
from
45f8225 to
1fa63ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
.env.example (1)
75-77: LGTM! Consider adding a brief explanation of the filter.The addition of the
SEEDING_WORKER_FILTERconfiguration aligns well with the PR objective of managing the seeding worker using environment variable-based filters. The JSON string is properly formatted.To improve clarity, consider adding a brief explanation of the purpose and possible values for this filter. For example:
# Sets the filter for the SeedingWorker -# SEEDING_WORKER_FILTER='{"isLayer1": true}' +# SEEDING_WORKER_FILTER='{"isLayer1": true}' # Filter to only seed Layer 1 data. Adjust the JSON object to set different filtering criteria.src/filters.ts (2)
167-175: LGTM! Consider adding a comment for clarity.The
MatchLayer1class is well-implemented and correctly identifies top-level items. It aligns with the PR objectives of enhancing seeding worker management.Consider adding a brief comment explaining the purpose of this class, for example:
export class MatchLayer1 implements ItemFilter { + // Matches items that are not nested (i.e., top-level or "layer 1" items) async match(item: MatchableItem): Promise<boolean> { return ( item.parent_id === '' || item.parent_id === null || item.parent_id === undefined ); } }
215-216: LGTM! Consider adding a type check for consistency.The addition of the
isLayer1condition is well-implemented and aligns with the PR objectives. It provides a way to create aMatchLayer1filter based on theisLayer1property.For consistency with other conditions in this function, consider adding a type check:
- } else if (filter?.isLayer1) { + } else if (filter?.isLayer1 === true) { return new MatchLayer1();This ensures that only a boolean
truevalue will trigger the creation of aMatchLayer1instance, matching the behavior of other conditions likeisNestedBundle.docker-compose.yaml (1)
102-102: LGTM. Consider updating documentation.The addition of the
SEEDING_WORKER_FILTERenvironment variable aligns with the PR objective of managing the seeding worker using environment variable-based filters. This change provides flexibility in configuring the seeding worker's behavior.Consider updating the project's documentation to include:
- The purpose and expected values of the
SEEDING_WORKER_FILTERvariable.- How this filter affects the seeding worker's behavior.
- Examples of common use cases for this filter.
This will help other developers understand and utilize this new configuration option effectively.
src/system.ts (2)
244-253: LGTM: Seeding worker logic implemented correctly.The new event handling logic for transaction indexing is well-implemented and aligns with the PR objectives. It correctly uses environment variable-based filters for managing the seeding worker.
Consider adding a null check for
seedingWorkerbefore calling itsseedmethod to prevent potential runtime errors:if ( seedingWorkerFilter !== undefined && tx.id !== undefined && (await seedingWorkerFilter.match(tx)) ) { - seedingWorker.seed(tx.id); + seedingWorker?.seed(tx.id); }
630-630: LGTM: SeedingWorker instance exported correctly.The seedingWorker instance is properly exported, allowing other modules to use it. The necessary dependencies (log and contiguousDataSource) are correctly passed to the constructor.
For consistency with other worker initializations in this file, consider moving this export statement closer to where other workers are initialized and exported. This would improve the overall organization of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
- .env.example (1 hunks)
- docker-compose.yaml (1 hunks)
- docs/envs.md (1 hunks)
- package.json (2 hunks)
- src/config.ts (1 hunks)
- src/filters.ts (2 hunks)
- src/system.ts (3 hunks)
- src/workers/seeding-worker.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/envs.md
- package.json
- src/config.ts
- src/workers/seeding-worker.ts
🔇 Additional comments (3)
src/filters.ts (1)
Line range hint
1-232: Overall, the changes look good and align with the PR objectives.The introduction of the
MatchLayer1class and the corresponding update to thecreateFilterfunction enhance the filtering capabilities for identifying top-level items. This aligns well with the PR objective of improving seeding worker management using environment variable-based filters.The implementation is clean, consistent with the existing code structure, and doesn't introduce any apparent issues. The minor suggestions provided earlier will further improve code clarity and consistency.
src/system.ts (2)
83-83: LGTM: SeedingWorker import added correctly.The import statement for SeedingWorker is properly formatted and follows the project's convention.
Line range hint
83-630: Overall: Changes look good and align with PR objectives.The implementation of the seeding worker is complete and integrates well with the existing codebase. The changes include:
- Proper import of the SeedingWorker class.
- Correct implementation of the event handling logic for transaction indexing.
- Export of the seedingWorker instance for use in other modules.
Minor suggestions were made for potential improvements, but these are not critical. The changes successfully introduce management of the seeding worker using environment variable-based filters as described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/workers/seeding-worker.ts (2)
24-44: LGTM: SeedingWorker class structure and constructor look good.The class declaration and constructor are well-implemented, initializing all necessary properties. However, consider adding error handling for the WebTorrent client initialization.
Consider wrapping the WebTorrent client initialization in a try-catch block:
constructor({ log, contiguousDataSource, fsDataStore, }: { log: Logger; contiguousDataSource: ContiguousDataSource; fsDataStore: FsDataStore; }) { try { this.webTorrentClient = new WebTorrent(); } catch (error) { this.log.error('Failed to initialize WebTorrent client', { error }); throw error; } // ... rest of the constructor }
1-67: Overall implementation aligns with PR objectives, with room for improvements.The SeedingWorker class successfully implements the core functionality described in the PR objectives. It integrates the WebTorrent library and provides a method for seeding data. The suggested improvements focus on enhancing error handling and Promise management, which will make the implementation more robust and maintainable.
Key points:
- The class structure and constructor are well-implemented.
- The seed method provides the core functionality but needs improvements in error handling and Promise management.
- Consider implementing the suggested changes to enhance the overall quality and reliability of the code.
As the project grows, consider implementing a more robust error handling strategy, possibly using custom error classes or a centralized error handling mechanism. This will help in better categorizing and responding to different types of errors that may occur during the seeding process.
src/system.ts (1)
245-253: LGTM: SeedingWorker filter logic implemented correctly.The implementation correctly checks for the existence of the filter and transaction ID before calling the
seedmethod. This ensures that seeding only occurs when necessary.Consider a minor optimization:
You could combine the conditions to avoid unnecessary method calls:
- if ( - seedingWorkerFilter !== undefined && - tx.id !== undefined && - (await seedingWorkerFilter.match(tx)) - ) { + if (seedingWorkerFilter && tx.id && await seedingWorkerFilter.match(tx)) { seedingWorker.seed(tx.id); }This change reduces the number of checks and improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/store/fs-data-store.ts (1 hunks)
- src/system.ts (4 hunks)
- src/workers/seeding-worker.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
src/workers/seeding-worker.ts (1)
1-22: LGTM: License and imports are correctly implemented.The license information is properly included, and the necessary imports for WebTorrent, Logger, FsDataStore, and ContiguousDataSource are present.
src/store/fs-data-store.ts (1)
47-49: Consider the implications of makingdataPathpublicChanging
dataPathfrom private to public exposes internal implementation details and could lead to several issues:
- It allows external code to bypass the
FsDataStoreabstraction, potentially leading to inconsistent data access.- It may create tight coupling between the storage implementation and other parts of the system.
- There's a potential security risk if the method is misused to access or modify files directly.
Could you provide more context on why this change was necessary? Are there other parts of the system that need direct access to file paths? If so, consider these alternatives:
- Create a new public method that provides only the necessary information without exposing the full path.
- Use the Observer pattern to notify interested parties about file locations without exposing the
dataPathmethod.To verify the usage of
dataPathin other parts of the codebase, you can run:This will help determine if the method is being called from other parts of the codebase, justifying the change to public.
✅ Verification successful
Verified:
dataPathmethod is used externally insrc/workers/seeding-worker.tsThe change to make
dataPathpublic is justified as it is utilized inseeding-worker.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of dataPath method outside this file rg --type typescript 'dataPath\(' -g '!src/store/fs-data-store.ts'Length of output: 103
Script:
#!/bin/bash # Search for usage of dataPath method outside src/store/fs-data-store.ts in TypeScript files rg 'dataPath\(' -g '*.ts' -g '!src/store/fs-data-store.ts'Length of output: 543
src/system.ts (4)
83-83: LGTM: SeedingWorker import added correctly.The import of
SeedingWorkeris properly placed with other worker imports and follows the project's naming conventions.
397-397: LGTM: fsDataStore correctly instantiated.The
fsDataStoreis properly created with the required parameters and placed before its usage in thecontiguousDataSourceconfiguration.
405-405: LGTM: contiguousDataSource updated to use fsDataStore.The
contiguousDataSourceconfiguration is correctly updated to use the newly createdfsDataStore. This change ensures consistency with the new SeedingWorker implementation.
632-636: LGTM: seedingWorker correctly instantiated and exported.The
seedingWorkeris properly created with the necessary parameters (log,contiguousDataSource, andfsDataStore) and exported for use in other modules. Its placement near other worker initializations is appropriate.
| async seed(txId: string) { | ||
| this.log.debug(`Seeding ${txId}`); | ||
| await this.contiguousDataSource.getData({ id: txId }); | ||
| const dataPath = this.fsDataStore.dataPath(txId); | ||
| await new Promise<void>((resolve) => | ||
| this.webTorrentClient.seed( | ||
| dataPath, | ||
| { | ||
| announce: [ | ||
| 'wss://tracker.btorrent.xyz', | ||
| 'wss://tracker.openwebtorrent.com', | ||
| 'wss://tracker.webtorrent.io', | ||
| ], | ||
| }, | ||
| (torrent: WebTorrent.Torrent) => { | ||
| this.log.debug(`Seeding ${txId} started: ${torrent.magnetURI}`); | ||
| resolve(); | ||
| }, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Improve error handling and Promise management in the seed method.
While the seed method implements the core functionality, there are several areas for improvement:
- Error handling: The method doesn't handle potential errors from
getDataordataPathoperations. - Promise handling: The method creates a new Promise but doesn't properly handle rejections.
- Async/await usage: The method is declared as async but doesn't use await consistently.
Consider refactoring the method as follows:
async seed(txId: string): Promise<void> {
this.log.debug(`Seeding ${txId}`);
try {
await this.contiguousDataSource.getData({ id: txId });
const dataPath = this.fsDataStore.dataPath(txId);
return new Promise<void>((resolve, reject) => {
this.webTorrentClient.seed(
dataPath,
{
announce: [
'wss://tracker.btorrent.xyz',
'wss://tracker.openwebtorrent.com',
'wss://tracker.webtorrent.io',
],
},
(torrent: WebTorrent.Torrent) => {
this.log.debug(`Seeding ${txId} started: ${torrent.magnetURI}`);
resolve();
}
).on('error', (error) => {
this.log.error(`Error seeding ${txId}`, { error });
reject(error);
});
});
} catch (error) {
this.log.error(`Error preparing data for seeding ${txId}`, { error });
throw error;
}
}This refactored version:
- Adds error handling for
getDataanddataPathoperations. - Properly handles Promise rejection for WebTorrent seeding errors.
- Uses async/await consistently.
- Returns the Promise to allow proper asynchronous handling by the caller.
| contiguousDataSource, | ||
| fsDataStore, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding seedingWorker to the shutdown process.
The seedingWorker has been correctly initialized and integrated into the system. However, it's not included in the shutdown process. To ensure proper cleanup and resource management, consider adding the seedingWorker to the shutdown function.
Add the following line to the shutdown function, along with the other worker shutdown calls:
await seedingWorker.stop();This will ensure that the seedingWorker is properly stopped when the system is shutting down.
