ubiquibot / permit-generation

A standalone module to generate permits.
0 stars 6 forks source link

Feat/rpc handler #19

Closed Keyrxng closed 2 days ago

Keyrxng commented 3 weeks ago

Resolves #18 Requires #17

gentlementlegen commented 2 weeks ago

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 commented 2 weeks ago

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

gentlementlegen commented 2 weeks ago

@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 commented 2 weeks ago

@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

Keyrxng commented 2 weeks ago

@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 commented 2 weeks ago

@Keyrxng Could you then try to:

Keyrxng commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 days ago

@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

rndquu commented 1 day ago

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

Keyrxng commented 1 day ago

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

Opened #29