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(gtest): make gtest thread safe #4032

Closed
wants to merge 6 commits into from
Closed

Conversation

clearloop
Copy link
Contributor

@clearloop clearloop commented Jun 24, 2024

the arch of gtest was forced to be synchronous since gear_lazy_pages should only be initialized for once ( if I'm not mistaken ), however since we have already checked the re-init problem at System::new, we actually can replace the lifetimes in the ExtManager related stuffs with smart pointers + locks to make gtest thread safe and more modular, for use case, see #4016

API Changes

since std::sync::RwLock will introduce error handling logic, so this PR uses parking_lot::RwLock instead so there's no api changes in the instance methods

  • lifetimes are removed

@gear-tech/dev

@clearloop clearloop added A0-pleasereview PR is ready to be reviewed by the team B2-breaking-apis A breaking change of which all stakeholders must be warned labels Jun 24, 2024
Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

  1. System must have only one instance per thread because of the global storages used under the hood. You see, gtest arch will be the following: it will use the same storage managing traits (Queue, Mailbox, TaskPool, ProgramStorage) and functions as on the pallet level. However, instaead of real storage, the in-memory global static ones will be used. System actually operates as the manager over these global storages through ExtManager. The latter actually stores in-memory per instance data, which is used to generate ids. If you have multiple instances of the System, you will have the issue with same ids being inserted into the same global storage (for example, same node ids in a global gas nodes storage). That's why currently we have singleton System. Maybe later after thorough redesign we will get rid of this rule.
  2. Please wait until refactoring(gtest): Introduce gear_commons mailbox to gtest #4010 is merged.
  3. Did I get it right that the PR is all about making the type system happy, as from the data perspective everything should be fine: because of the thread_local System has only one instance per thread, each thread has it's own copy of the System (see tests).
  4. Are you sure that's a good time for Connect gclient with gtest #3895? gclient is currently under the refactoring/introducing the proper implementation of the runtime features.

@clearloop
Copy link
Contributor Author

clearloop commented Jul 3, 2024

  1. Did I get it right that the PR is all about making the type system happy, as from the data perspective everything should be fine: because of the thread_local System has only one instance per thread, each thread has it's own copy of the System (see tests).

with Arc<RwLock>, we can embed System in other instance without lifetimes and can use it in async functions

  1. Are you sure that's a good time for Connect gclient with gtest #3895? gclient is currently under the refactoring/introducing the proper implementation of the runtime features.

As for #3895, I'm about to only wrap the basic interfaces like deploy, send, read_state with the high level interfaces of gtest and gclient, just to verify everything works first, and the new instance that PR introduced will only be available with features like expermental

@techraed
Copy link
Member

techraed commented Jul 7, 2024

with Arc, we can embed System in other instance without lifetimes and can use it in async functions

Alright. I'd state that wherever we embed the System, we must be sure that it's usage is in accordance to the design and intented usage practices for the System (singleton, instantiated only once with non-fallible function, thread safe as it has a copy in each thread).

@breathx breathx requested a review from techraed July 7, 2024 16:16
@breathx
Copy link
Member

breathx commented Jul 7, 2024

Blocked by 4010

@breathx breathx added the A5-dontmerge PR should not be merged yet label Jul 7, 2024
@techraed
Copy link
Member

techraed commented Jul 9, 2024

@clearloop will try to merge #4010 by the end of the week

@techraed techraed self-requested a review July 9, 2024 19:17
@techraed
Copy link
Member

would like to review the PR after resolving confilcts

@clearloop clearloop added A1-inprogress Issue is in progress or PR draft is not ready to be reviewed and removed A0-pleasereview PR is ready to be reviewed by the team labels Jul 19, 2024
@clearloop
Copy link
Contributor Author

clearloop commented Jul 23, 2024

need a better solution since lazy_pages is based on thread_local! as well, make System thread safe without modifying lazy_pages may encounter problems like: System is sync but lazy_pages is not

@clearloop clearloop closed this Jul 23, 2024
@clearloop clearloop deleted the cl/ts-gtest branch September 28, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1-inprogress Issue is in progress or PR draft is not ready to be reviewed A5-dontmerge PR should not be merged yet B2-breaking-apis A breaking change of which all stakeholders must be warned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants