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

refactor: on-demand remote pin status checks #1903

Merged
merged 4 commits into from
Feb 11, 2022

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 9, 2022

This is bare minimum to restore remote pinning with rate-limited Pinata and close #1900
Not a perfect solution, but a fast-tracked fix to restore pinning functionality at all (right now Pinata is effectively broken in webui and ipfs-desktop – details in #1900 (comment)).

Key changes:

  • Files screen does not make automatic requests to remote pinning service every time a new directory is opened
    • we progressively build a cache with remote pin states every time user opens "Set pinning" modal, and send requests only for files that we did not check the remote state before
    • we persist the pinning bundle state with remote pin states with createCacheBundle (src/bundles/index.js) and persistActions (src/bundles/pinning.js), which store data in browser's IndexedDB (uses money-clip as cache backend)
    • new users should not see any regression: the pin would be added to the cache during pinning operation, and thanks to persisted cache, cloud pin will show every time Files screen loads.
    • old users may need to manually go to "Set pinning" to trigger status fetch, but that is the best we can do and a fair compromise, given nothing worked before this.
  • The expensive ipfs pin remote service ls --stat checks are executed only on Settings screen – Files does not need them.
  • Remote pin status check is triggered by opening "Set pinning" modal via context menu or the pin icon, or by clicking on "Pin Status" column header.
    • Clicking on "Pin Status" header bypassess remote pin status cache and forces online status re-check for all items in current directory (bit of a hidden feature / escape hatch for frustrated, existing users)
  • Errors returned by pinning service are now correctly reported to the user. The CLI and ipfs-webui users will now see the same error message:

    2022-02-09_17-41_1
    2022-02-09_17-41

This is bare minimum to restore remote pinning with rate-limited Pinata.
Details in #1900 (comment)

- The expensive 'ipfs pin remote service ls --stat' and autopin MFS policy
  is executed only on Settings screen

- Files screen does not make any requests to remote pinning service anymore.

- Remote pin status check is triggered by opening "Set pinning"
  modal via context menu or the pin icon, or by clicking on "Pin Status"
  column header.

– We cache aggressively and assume pins on remote service are not changed
  by other means. The cache lives in memore, reloading the webui will
  purge cached states.

- Clicking on "Pin Status" header bypassess remote pin status cache and
  forces online status re-check for all items in current directory.

- Errors returned by pinning service are now correctly reported to the
  user. The CLI and ipfs-webui users will now see the same error
  message.
notPins.delete(pinCid)
adds.push({ id: `${service.name}:${pinCid}`, ...pin })
try {
// TODO: execute remote.ls with +1m backoff per service when response Type == "error" and Message includes "429 Too Many Requests"
Copy link
Member Author

@lidel lidel Feb 9, 2022

Choose a reason for hiding this comment

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

ℹ️ Curiously, hardcore caching is enough to send less than 6 requests per minute (I was not able to trigger rate-limiting at Pinata).

👉 What happens on 429 Too Many Requests?

If the user overdoses caffeine and pins more than 6 things in one minute, they will get a meaningful error via notify.js (yellow Toaster stating):

Unable to set Pinning at Pinata: Error: reason: "Rate limit exceeded", details: "You have exceeded the API rate limit. Wait a minute and slow down your requests.": 429 Too Many Requests

Having this, I decided to not implement any throttling on the client. There is no point in ipfs-desktop or go-ipfs trying to hide the throttling by visually slowing pin checks down – that would give an impression of "IPFS Desktop being slow" while it is remote HTTP server's limitation.

But it could be better!

I agree! If anyone is interested in improving remote pinning implementation, a good starting point is #1752 where existing technical debt is described, and a clean way to rewrite it. Figuring out UX for handling rate-limiting (HTTP 429) would be a small part of that work (e.g. option for user to "dismiss & keep trying in background")

@lidel lidel temporarily deployed to Deploy February 9, 2022 20:01 Inactive
this leverages createCacheBundle (src/bundles/index.js)
and persistActions (src/bundles/pinning.js) for persisting remote pin
states across page reloads

new users should not see any functional difference, old users may need
to manually go to "Set pinning" to trigger status fetch, but that is the
best we can do and a fair compromise, given that before this remote pins
at Pinata did not work at all due to throttling
(#1900)
@lidel lidel temporarily deployed to Deploy February 11, 2022 01:41 Inactive
we added two variables, and old key had none.
using a different key ensures users use english instead of old
translation
@lidel lidel marked this pull request as ready for review February 11, 2022 02:00
@lidel lidel temporarily deployed to Deploy February 11, 2022 02:04 Inactive
@lidel lidel temporarily deployed to Deploy February 11, 2022 02:19 Inactive
@lidel
Copy link
Member Author

lidel commented Feb 11, 2022

Did as many tests I could. Fast-tracking this, as we need to restore remote pinning on Pinata in go-ipfs 0.13 and ipfs-desktop 0.19.

@lidel lidel merged commit 10f21e9 into main Feb 11, 2022
@lidel lidel deleted the fix/pinning-service-rate-limiting branch February 11, 2022 13:26
Copy link

@CryptoUnchained CryptoUnchained left a comment

Choose a reason for hiding this comment

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

``

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.

Pinning to Pinata disabled due to "Rate limit exceeded"
2 participants