Block the use of IForegroundNotificationService when there is no foreground thread#25938
Conversation
There was a problem hiding this comment.
don't we have test foreground notification service?
There was a problem hiding this comment.
Yes we should remove that but it's still on the backlog.
There was a problem hiding this comment.
so. we no longer use mock on tests? is that it?
There was a problem hiding this comment.
Is it safe to silently discard these? Maybe a non-fatal error could be logged.
There was a problem hiding this comment.
cc @KirillOsenkov we used to have no logging for these notifications and we had them silently discarded. We need to verify we're doing the right thing here if this patch goes in.
There was a problem hiding this comment.
@Therzok I'm not sure what the specific concern is here. With this change, it is impossible to queue work without a foreground thread, hence why I didn't start the task.
There was a problem hiding this comment.
From what I see, the fallback thread data will not queue work. The concern is needing to know which things need to be setup to properly function.
We had code throwing in this service, that was masked, because we didn't have FatalError handlers setup.
Maybe it's worth notifying the consumer that the setup is not properly done?
As another point, I was thinking about filing a bug about FatalError not saying no handler is registered. Maybe ForehroundNotificationService should report that no thread handler is set?
There was a problem hiding this comment.
From what I see, the fallback thread data will not queue work.
Correct. It will now throw a synchronous exception immediately upon attempting to schedule the work.
There was a problem hiding this comment.
Ohhhh, I just noticed the Contract.ThrowIfTrue calls below. No worries from my side anymore!
There was a problem hiding this comment.
I am changing this test to wait for semantic change processor. leave the fix to me.
63d638d to
cdb05e1
Compare
|
so I guess when testing features that require foreground thread in unit test, we need to mark the unit test WpfTest or something? |
|
@sharwell I am fine with change. but if unit test that require certain attribute if it needs foreground thread for whatever reason, it would be nice if there is some doc or wiki people can learn about our unit test magics. |
This was already the case, but breaking the rule in the past would typically just make for a flaky test. |
|
I'm hesitant about treating this as a test only change considering the number of times changes like this have broken scenarios like |
|
@sharwell shouldn't it then explicitly crash or throw with message saying [WpfTest] should be marked on test that require UI? |
Technically this isn't the requirement. The requirement is that a "main thread" of some form be available which is not also a thread pool thread. Generally code which violates this will hit a different problem much earlier by accessing a foreground thread affinitized object that asserts in the constructor. This service is a special case because it needs to support construction when no foreground thread is available, provided work is not queued in it. |
|
wouldn't it be safer to just asking any tests that require main thread to just always marked as [WpfFact] rather than one to know all these details whether [WpfFact] is okay to omitted in certain case? if (ForegroundKind != ForegroundThreadDataKind.Unknown) not sure how many people in our team actually know all the implication of ThreaddataKind being different ones. |
|
@heejaechang The general approach is adding a call to Improved messaging around this is something we can add in the future, perhaps as part of #25805. Also, keep in mind that it's quite possible other tests that use |
Customer scenario
A correctly-written test could incorrectly fail during a build.
Bugs this fixes
Fixes #25930
Workarounds, if any
None
Risk
Low. The conditions under which behavior is changed do not occur in production environments.
Performance impact
Low. A new assertion is added on a code path that is only called when work is being queued on a different thread.
Is this a regression from a previous update?
Yes.
Root cause analysis
This is a race condition where the following sequence occurs:
[UseExportProvider]but does not useIForegroundNotificationService[WpfFact]UseExportProviderAttribute.After, an instance ofForegroundNotificationServiceis constructed here, which queues a background taskUseExportProviderAttribute.Afteris executed on a background thread, and reaches this join operation before the task starts runningThis change prevents the
IForegroundNotificationServicefrom being used to schedule foreground operations when there is no foreground thread, and then resolves the source of the crash in this case by avoiding the creation of the background operation.How was the bug found?
Automated testing
Test documentation updated?
N/A