-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Move the initialization of the global notification service #80062
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
Move the initialization of the global notification service #80062
Conversation
4152a4e to
cfe00ec
Compare
AbhitejJohn
left a comment
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.
Would we need to validate Source Based Discovery (which I believe is the unit testing component that needs the old solution crawler) for this change?
|
@AbhitejJohn: probably, or at least stick this under a debugger and validate that the same registration and event hookup code is running. Since this is just a straight code move with no effort to clean it up I'm not too worried. |
We were initializing this in package load, because at one point the APIs to do so had to be on the UI thread. At this point the only remaining use of this service is for the legacy solution crawler, so there's no point in initializing it until the service is actually constructed. And since everything is now free threaded, it's easy to move.
cfe00ec to
e341388
Compare
|
Turns out this code isn't actually being used. Starting a conversation with a few folks. |
|
It looks like #74239 removed the service that proxied this information over to ServiceHub; without that implementation this is dead code. I'm going to go ahead and merge this PR just because we're right now wasting time on load running this, but we'll still decide on a long term plan. |
CyrusNajmabadi
left a comment
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.
LGTM.
We were initializing this in package load, because at one point the APIs to do so had to be on the UI thread. At this point the only remaining use of this service is for the legacy solution crawler, so there's no point in initializing it until the service is actually constructed. And since everything is now free threaded, it's easy to move.