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

Feat/rpc handler #19

Merged
merged 6 commits into from
Jul 6, 2024
Merged

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Jun 16, 2024

Resolves #18
Requires #17

@Keyrxng Keyrxng marked this pull request as draft June 16, 2024 22:57
@Keyrxng Keyrxng marked this pull request as ready for review June 18, 2024 00:03
@gentlementlegen
Copy link
Contributor

Tests seem to works but some of them do not exit properly, I suspect some import containing async calls that is not awaited, see here.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 18, 2024

Tests seem to works but some of them do not exit properly, I suspect some import containing async calls that is not awaited, see here.

running -detectOpenHandles locally shows no leaks and removes the warning. That warning is consistent when testing the rpc-handler see here and I have spent some time in trying to resolve it but haven't been able to beyond using the flag. I've tried rpc call timeouts, cancelTokens, fetch and axios, increasing test timeouts to silly values, used unref(), etc but nothing seems to resolve it completely every time

Co-authored-by: Mentlegen <9807008+gentlementlegen@users.noreply.github.com>
@gentlementlegen
Copy link
Contributor

@Keyrxng A common scenario I encountered is when you import a file (import 'my-file') and within this file some function is directly called such as:

run().then().catch(e => {})

This mean on import, async functions run instantly but are not awaited and run beyond the tear down of Jest, if that helps you.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 18, 2024

@gentlementlegen I understand what you mean and why that would be the case but there isn't anything like that in either of the two codebases in this case I've just had a look through them. Theres main.ts in this repo but it's not involved in tests.

Unless the rpc-handler bundled somehow ends up with something like that in it which I don't think should happen I'm not sure that's the cause in this case but that's a good thing to keep an eye out for ty.

I think it's directly related to the racing of the RPCs from what I've seen but like I said I've not been able to remove the warning 100% of the time


You don't see the error in pay.ubq.fi tests only in jest tests do we see it. Using unref() broke the RPC racing in the browser so I reverted that, I think it's node/jest + RPC racing related but pinpointing the cause has eluded me

@@ -0,0 +1,26 @@
// @ts-expect-error - no types
Copy link
Contributor

Choose a reason for hiding this comment

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

rpc-handler seems to have types?

Copy link
Member Author

Choose a reason for hiding this comment

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

The repo does but the package does not for some reason. My test pkg I published has types and it's the same flow that was used to publish but the UBQ pkg doesn't have types in either of the versions on npmjs

https://www.npmjs.com/package/@ubiquity-dao/rpc-handler?activeTab=code

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 18, 2024

@Keyrxng A common scenario I encountered is when you import a file (import 'my-file') and within this file some function is directly called such as:

run().then().catch(e => {})

This mean on import, async functions run instantly but are not awaited and run beyond the tear down of Jest, if that helps you.

the rpc-handler tests with --detectOpenHandles shows this twice, which is part of axios and occassionally it might point to where the cancelToken is used as part of the rpcTimeout

Jest has detected the following 2 open handles potentially keeping Jest from exiting: 

  ●  TLSWRAP

      16423 |       withCredentials
      16424 |     });
    > 16425 |     let response = await fetch(request);
            |                          ^
      16426 |     const isStreamResponse = supportsResponseStream && (responseType ===
 "stream" || responseType === "response");
      16427 |     if (supportsResponseStream && (onDownloadProgress || isStreamResponse)) {
      16428 |       const options = {};
      ```

@gentlementlegen
Copy link
Contributor

@Keyrxng Could you then try to:

  • hard code these calls and see if that solves the issue
  • if it does, might be better to mock these for testing

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 19, 2024

@gentlementlegen I've mocked the handler and return a valid provider instead of racing them. Tests could be refactored to use a local anvil instance like other projects that rely on on-chain interactions if this impl is a problematic approach to tests.

The warning is no longer present so the issue stems from rpc-handler. Because all tests pass in that repo I was under the impression that while you'd rather not have the warning it's not that big of a problem but if it is a problem I can spend some more time and find a solution for it.

@gentlementlegen
Copy link
Contributor

I think this should be solved, because at the moment you just get a warning that some code is running after Jest tear down, but if that code was actually outputting things using console.log Jest would throw an error saying some code is attempting to log after tear down.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 20, 2024

Resolved in rpc-handler and I've linked the updated package locally and it shows the tests passing without the warning present so the last commit will be reverted and after proxify is merged things should be fixed.

image

@0x4007
Copy link
Member

0x4007 commented Jul 6, 2024

@rndquu Wrote the spec and approved the pull so I think it's an automatic accept

@gentlementlegen We should consider this for the auto pull merge plugin

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Implicit ok

@0x4007 0x4007 merged commit 4c97215 into ubiquibot:development Jul 6, 2024
2 checks passed
@rndquu
Copy link
Contributor

rndquu commented Jul 7, 2024

@Keyrxng Could you fix https://github.com/ubiquibot/permit-generation/actions/runs/9821721152/job/27117704115 ?

@Keyrxng
Copy link
Member Author

Keyrxng commented Jul 7, 2024

@Keyrxng Could you fix https://github.com/ubiquibot/permit-generation/actions/runs/9821721152/job/27117704115 ?

Opened #29

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.

Use @ubiquity-dao/rpc-handler instead of hardcoded RPC urls
5 participants