-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 globalstatic
ones will be used.System
actually operates as the manager over these global storages throughExtManager
. The latter actually stores in-memory per instance data, which is used to generate ids. If you have multiple instances of theSystem
, 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 singletonSystem
. Maybe later after thorough redesign we will get rid of this rule.- Please wait until refactoring(gtest): Introduce
gear_common
s mailbox togtest
#4010 is merged. - 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 theSystem
(see tests). - 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.
with
As for #3895, I'm about to only wrap the basic interfaces like |
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 |
Blocked by 4010 |
@clearloop will try to merge #4010 by the end of the week |
would like to review the PR after resolving confilcts |
need a better solution since lazy_pages is based on |
the arch of
gtest
was forced to be synchronous sincegear_lazy_pages
should only be initialized for once ( if I'm not mistaken ), however since we have already checked there-init
problem atSystem::new
, we actually can replace the lifetimes in theExtManager
related stuffs with smart pointers + locks to make gtest thread safe and more modular, for use case, see #4016API Changes
@gear-tech/dev