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

Pics Cache Enhancements #389

Closed

Conversation

Revadike
Copy link
Contributor

@Revadike Revadike commented Mar 9, 2022

  • Allow pics cache to be saved to disk (Custom cache engine for picsCache #188)
  • Read (and sync) saved pics cache to reduce requesting productinfos (Cache for appOwnershipCached #385)
  • Refactor code and reduce complexity of getProductInfo() (comments 1 2)
  • Request product info in bulk (and save progress in between).
  • Request missing tokens by default? (discussion)
  • Use meta_data_only instead of tracking changenumber (reason: changenumber changes can be only requested until a certain point)
  • Fix README.md

@Revadike
Copy link
Contributor Author

Revadike commented Mar 9, 2022

@DoctorMcKay early feedback is welcome and encouraged!

@DoctorMcKay
Copy link
Owner

Request missing tokens by default?

It does make sense to request missing tokens by default, but this would be a breaking change so it's best not to.

resources/default_options.js Show resolved Hide resolved
README.md Outdated

Defaults to `false`.

**Warning:** This will significantly increase the storage space that is needed!
Copy link
Owner

Choose a reason for hiding this comment

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

How much disk space does this need? Might it be worthwhile to gzip the json before we store it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should add "potentially", because it all depends on how many products you will be requesting. But it will build up over time, of course.
As for your first question, I once saved the entire SteamUser instance to disk as JSON. This was around 140 MB and the majority of that data consisted of the pics cache JSON. That was 4 years ago and my library has grown from then. Anyway, it will be in the order of hundreds of MB. Maybe getting into the gigabytes eventually, if you are going to monitor all pics changes. Using the custom file engine, one may implement a database hook instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, with the main functionality added now, I got this by just saving my owned products infos to disk:
image

I think that's reasonable, especially with my unreasonable library size :P
People can always choose to not save pics cache to disk anyway.
The danger lies if people also use picsCacheAll, and potentially save product infos of everything. I've added a warning especially for this.

@Revadike
Copy link
Contributor Author

Revadike commented Mar 9, 2022

Request missing tokens by default?

It does make sense to request missing tokens by default, but this would be a breaking change so it's best not to.

Hmm, I'm thinking, maybe just like the global ownershipFilter, the same can be done by globally setting this behavior too.

@DoctorMcKay
Copy link
Owner

Hmm, I'm thinking, maybe just like the global ownershipFilter, the same can be done by globally setting this behavior too.

Ehh, I'm not a big fan of adding a new global option just to avoid having to type true a few times. A global ownership filter makes sense because it affects many different methods and is a complex data type, but a simple bool isn't a burden to type.

@Revadike
Copy link
Contributor Author

Revadike commented Mar 9, 2022

Fair enough

@Revadike Revadike marked this pull request as ready for review May 22, 2022 00:52
@Revadike Revadike requested a review from DoctorMcKay May 22, 2022 00:52
@Revadike
Copy link
Contributor Author

It should work as-is, but for this PR to work better, I highly suggest merging this PR (and potentially this PR) as well.

@@ -137,7 +137,7 @@ class SteamUserApps extends SteamUserAppAuth {
* Get a list of apps or packages which have changed since a particular changenumber.
* @param {int} sinceChangenumber - Changenumber to get changes since. Use 0 to get the latest changenumber, but nothing else
* @param {function} [callback]
* @returns {Promise<{currentChangeNumber: number, appChanges: number[], packageChanges: number[]}>}
* @returns {Promise<{currentChangeNumber: number, appChanges: object[], packageChanges: object[]}>}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong, btw. I fixed it.


for (let appid in apps) {
// Public only apps are weird...
if (apps[appid].missingToken && !apps[appid].appinfo.public_only) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really conflicted about this and would love your input. Should I save product infos with missing tokens?
Initially, I would say yes, but that will lead to a difficult to handle situation: when you activate a new product on steam, it will not update your cached product info. Currently, it only updates saved pics cache when changes have been detected since last synced changenumber.
Additionally, there is the case of public_only apps. These are weird, since even if you own it, you cannot seem to request product access tokens for them?

// This one actually can take a while, so allow it to go as long as 60 minutes
return StdLib.Promises.timeoutCallbackPromise(3600000, ['apps', 'packages', 'unknownApps', 'unknownPackages'], callback, (resolve, reject) => {
// This one actually can take a while, so allow it to go as long as 120 minutes
return StdLib.Promises.timeoutCallbackPromise(7200000, ['apps', 'packages', 'unknownApps', 'unknownPackages'], callback, async (resolve, reject) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Increased to 2h, because one of the top game owners on steam is trying to use node-steam-user but has issues getting passed this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Increased to 2h, because one of the top game owners on steam is trying to use node-steam-user but has issues getting passed this point.

Btw, I can confirm with all my changes, this person was able to finally get his ownership cached. 🎉🎉

@Revadike
Copy link
Contributor Author

Revadike commented Jun 11, 2022

New TODO:

  • Use meta_data_only instead of tracking changenumber (reason: changenumber changes can be only requested until a certain point)

@Revadike Revadike force-pushed the feature-picscache-enhancements branch from b92130b to 3249f29 Compare June 17, 2022 04:06
@Revadike
Copy link
Contributor Author

Revadike commented Jun 17, 2022

New TODO:

  • Fix README.md

}

if (this.options.picsCacheAll && this.options.savePicsCache) {
this._warn('Both picsCacheAll and savePicsCache are enabled. Unless a custom storage engine is used, beware that this will cause a lot of disk IO and space usage!');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's a good idea to detect custom storage usage, somehow...


If `enablePicsCache` is enabled, saves all product info from [the PICS cache](#picscache) to disk (in [`dataDirectory`](#dataDirectory)) or to your [Custom Storage Engine](#custom-storage-engine). This will significantly speed up the [`appOwnershipCached`](#appOwnershipCache) event from firing and reduce the amount product info requests to Steam. It will only save product info if it's not missing its access token.

Added in {{TODO}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pending version bump

// "pkg cache:", Object.keys(cached.packages).length,
// "app to refresh:", apps.length,
// "pkg to refresh:", packages.length,
// );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I keep this? It's a great log for debugging purposes.

@Revadike
Copy link
Contributor Author

Revadike commented Jul 1, 2022

Bug found: requesting product info of subids/appids as string results in deadlock.

Edit:
Or maybe not? :S

@Revadike
Copy link
Contributor Author

Ready to merge?

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