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.

Sign up for GitHub

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

Always try to register the EditorConfigDocumentOptionsProvider #40513

Merged
merged 10 commits into from
Mar 2, 2020
Merged

Always try to register the EditorConfigDocumentOptionsProvider #40513

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ec43652
Always try to register the EditorConfigDocumentOptionsProvider
JoeRobich Dec 19, 2019
55acf88
Remove import of VisualStudioWorksapce
JoeRobich Jan 8, 2020
e56bf8e
Remove unnecessary comment
JoeRobich Jan 8, 2020
007b21b
Make IErrorLoggerService not required
JoeRobich Jan 8, 2020
ccdc697
Add MockOptionService for test
JoeRobich Jan 8, 2020
25ea1db
Mark MockOptionService.OptionChanged event as unused
JoeRobich Jan 8, 2020
ec1ce4d
Merge remote-tracking branch 'origin/master' into workspace-registers…
JoeRobich Jan 8, 2020
505d448
Merge branch 'master' into workspace-registers-optionsprovider
JoeRobich Jan 23, 2020
f2d5c95
Ensure OptionPersisters are loaded before creating a workspace
JoeRobich Feb 21, 2020
48bf4d9
Avoid invoking GetOptions from Workspace constructor
mavasani Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions ...rkspaces/Core/Portable/Options/EditorConfig/EditorConfigDocumentOptionsProviderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,16 @@

using System;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.ErrorLogger;

namespace Microsoft.CodeAnalysis.Options.EditorConfig
{
[Export(typeof(IDocumentOptionsProviderFactory)), Shared]
[ExportMetadata("Name", PredefinedDocumentOptionsProviderNames.EditorConfig)]
internal sealed class EditorConfigDocumentOptionsProviderFactory : IDocumentOptionsProviderFactory
internal static class EditorConfigDocumentOptionsProviderFactory
{
[ImportingConstructor]
public EditorConfigDocumentOptionsProviderFactory()
{
}

public IDocumentOptionsProvider? TryCreate(Workspace workspace)
public static IDocumentOptionsProvider? TryCreate(Workspace workspace)
mavasani marked this conversation as resolved.
Show resolved Hide resolved
{
if (!ShouldUseNativeEditorConfigSupport(workspace))
Copy link
Contributor

@mavasani mavasani Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@JoeRobich I was able to verify the integration test failures from this PR locally, and the root cause seems to be this code path (from workspace constructor) can be invoked from a background thread, but fetching options can force loading of option persisters, which seem to require UI thread on creation. I moved this specific check ShouldUseNativeEditorConfigSupport from here down into GetOptionsForDocumentAsync below and then the tests pass locally. I think the correct long term solution here would be to remove the UI thread requirement from option persisters during their construction OR add functionality in GlobalOptionsService to be able to force UI thread before loading persisters. For now, this change should unblock us.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, do we still need to support legacy editorconfig provider? I would be fine with us deleting it completely as well. Tagging @jasonmalinowski

{
Expand Down
15 changes: 15 additions & 0 deletions src/Workspaces/Core/Portable/Workspace/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.ErrorLogger;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Options.EditorConfig;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -74,6 +76,19 @@ protected Workspace(HostServices host, string? workspaceKind)

// initialize with empty solution
_latestSolution = CreateSolution(SolutionId.CreateNewId());

_taskQueue.ScheduleTask(() => TryRegisterDocumentOptionsProvider(), nameof(TryRegisterDocumentOptionsProvider));
Copy link
Member

Choose a reason for hiding this comment

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

  1. why on a task? are there race conditions here?
  2. what general value is this change trying to provide? What's wrong with MEF here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked to @jasonmalinowski offline and he helped me get to the root of the issue. Will update in a bit. Thanks for taking a look!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Thanks!

}

internal void TryRegisterDocumentOptionsProvider()
{
var documentOptionsProvider = EditorConfigDocumentOptionsProviderFactory.TryCreate(this);
if (documentOptionsProvider is null)
{
return;
}

_workspaceOptionService?.RegisterDocumentOptionsProvider(documentOptionsProvider);
}

internal void LogTestMessage(string message)
Expand Down