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

Only create api promise once #249

Conversation

TorstenStueber
Copy link
Collaborator

Closes #248

This solution avoids the memory leak and even after running for many requests, the heap will regularly be completely garbage collected.

However, I don't know whether this solution works reliably over a long time. I just tested locally and disconnected WIFI repeatedly and the service could successfully reconnect again. Unfortunately, I can't find any documentation for these functions.

@TorstenStueber TorstenStueber linked an issue Oct 31, 2024 that may be closed by this pull request
Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for pendulum-pay ready!

Name Link
🔨 Latest commit 5b13e2b
🔍 Latest deploy log https://app.netlify.com/sites/pendulum-pay/deploys/6724005a27092300084d385a
😎 Deploy Preview https://deploy-preview-249--pendulum-pay.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@TorstenStueber TorstenStueber requested a review from a team October 31, 2024 22:11
await api.isReady;

const currentSpecVersion = await getSpecVersion();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ensure that we reload all metadata after a runtime upgrade.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

In other services we already have a caching mechanism in place. It's implemented with this class. Since we are only dealing with one network (pendulum) in the signer-service, I and don't need to cache the API per network, I think it's fine the way you implemented it.

@TorstenStueber
Copy link
Collaborator Author

Thanks @ebma, I didn't even check how we do it elsewhere. Just looking at the other code, they don't reconnect the api in case it becomes disconnected. I didn't expect this to work.

I was just testing this case locally. Actually when the connection fails and you wait long enough, polkadot-js just crashes the complete app, so that it is forced to restart completely anyway. This is exactly not what I expect a library to do. 😱

So ignore my concern, the other codebases will work then.

@TorstenStueber TorstenStueber merged commit 82920da into polygon-prototype-staging Nov 1, 2024
5 checks passed
@TorstenStueber TorstenStueber deleted the 248-bug-memory-leak-in-signer-service branch November 1, 2024 10:58
@ebma
Copy link
Member

ebma commented Nov 1, 2024

Just looking at the other code, they don't reconnect the api in case it becomes disconnected. I didn't expect this to work.

It does actually. In the testing-service, if you have a look here, the API is reconnected when a call fails (which might fail because the API is disconnected, or because the metadata is outdated, etc). The same logic is in the token-api here and in the faucet here.

Actually when the connection fails and you wait long enough, polkadot-js just crashes the complete app, so that it is forced to restart completely anyway. This is exactly not what I expect a library to do. 😱

That's crazy though, does it throw a specific error or how does it crash?

@TorstenStueber
Copy link
Collaborator Author

TorstenStueber commented Nov 1, 2024

It does actually.

True, I missed that.

does it throw a specific error

The error is:

2024-11-01 07:55:17        RPC-CORE: call(method: Text, data: Bytes, at?: BlockHash): Bytes:: No response received from RPC endpoint in 60s
<redacted>/signer-service/node_modules/@polkadot/rpc-provider/cjs/ws/index.js:497
                    handler.callback(new Error(`No response received from RPC endpoint in ${this.__internal__timeout / 1000}s`), undefined);
                                     ^

Error: No response received from RPC endpoint in 60s
    at WsProvider.__internal__timeoutHandlers (<redacted>/signer-service/node_modules/@polkadot/rpc-provider/cjs/ws/index.js:497:38)
    at Timeout._onTimeout (<redacted>/signer-service/node_modules/@polkadot/rpc-provider/cjs/ws/index.js:178:65)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)

Node.js v21.7.0

Having a closer look at this, it might not be polkadot-js to blame but we possibly just miss some proper catch blocks around any calls to the polkadotjs Api object, the way it is done in the other codebases.

Anyway, an error thrown inside a handler of an express app should usually not crash the whole app but should be caught by express so that it will just respond with a 500 error. That's why it looks strange to me.

@ebma
Copy link
Member

ebma commented Nov 1, 2024

Hmm okay, weird. I guess it's because of this timeout. It's 60 seconds by default but we can pass another one. Looking at this, every call we do is stored as a handler and these handlers are looped to check if any of them timed out. Seems like handlers are only created for each call to send() and the API wouldn't time out just like that without being used 🤔

@TorstenStueber
Copy link
Collaborator Author

I realized that we configured the express server wrong and the error handler is not called properly so that the whole app crashes instead of returning a 500. I just fixed this in another PR.

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.

Bug: Memory Leak in signer service
2 participants