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

Fixes bugs when there are multiple handlers are defined for a language for a given request #1582

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 7 additions & 0 deletions src/OmniSharp.Abstractions/Models/ICanBeEmptyResponse.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
namespace OmniSharp.Models
{
public interface ICanBeEmptyResponse
dmgonch marked this conversation as resolved.
Show resolved Hide resolved
{
bool IsEmpty { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@

namespace OmniSharp.Models.GotoDefinition
{
public class GotoDefinitionResponse
public class GotoDefinitionResponse : ICanBeEmptyResponse
dmgonch marked this conversation as resolved.
Show resolved Hide resolved
{
public string FileName { get; set; }
[JsonConverter(typeof(ZeroBasedIndexConverter))]
public int Line { get; set; }
[JsonConverter(typeof(ZeroBasedIndexConverter))]
public int Column { get; set; }
public MetadataSource MetadataSource { get; set; }
public bool IsEmpty => FileName == null || FileName == string.Empty;
}
}
1 change: 1 addition & 0 deletions src/OmniSharp.Host/AssemblyInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@
[assembly: InternalsVisibleTo("OmniSharp.Http.Tests")]
[assembly: InternalsVisibleTo("OmniSharp.MSBuild.Tests")]
[assembly: InternalsVisibleTo("OmniSharp.Roslyn.CSharp.Tests")]
[assembly: InternalsVisibleTo("OmniSharp.Stdio.Tests")]
[assembly: InternalsVisibleTo("TestUtility")]
86 changes: 73 additions & 13 deletions src/OmniSharp.Host/Endpoint/EndpointHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public class EndpointHandler<TRequest, TResponse> : EndpointHandler
{
private readonly CompositionHost _host;
private readonly IPredicateHandler _languagePredicateHandler;
private readonly Lazy<Task<Dictionary<string, ExportHandler<TRequest, TResponse>>>> _exports;
private readonly Lazy<Task<Dictionary<string, ExportHandler<TRequest, TResponse>[]>>> _exports;
private readonly OmniSharpWorkspace _workspace;
private readonly bool _hasLanguageProperty;
private readonly bool _hasFileNameProperty;
Expand All @@ -71,10 +71,10 @@ public EndpointHandler(IPredicateHandler languagePredicateHandler, CompositionHo
_canBeAggregated = typeof(IAggregateResponse).IsAssignableFrom(metadata.ResponseType);
_updateBufferHandler = updateBufferHandler;

_exports = new Lazy<Task<Dictionary<string, ExportHandler<TRequest, TResponse>>>>(() => LoadExportHandlers(handlers));
_exports = new Lazy<Task<Dictionary<string, ExportHandler<TRequest, TResponse>[]>>>(() => LoadExportHandlers(handlers));
}

private Task<Dictionary<string, ExportHandler<TRequest, TResponse>>> LoadExportHandlers(IEnumerable<Lazy<IRequestHandler, OmniSharpRequestHandlerMetadata>> handlers)
private Task<Dictionary<string, ExportHandler<TRequest, TResponse>[]>> LoadExportHandlers(IEnumerable<Lazy<IRequestHandler, OmniSharpRequestHandlerMetadata>> handlers)
{
var interfaceHandlers = handlers
.Select(export => new RequestHandlerExportHandler<TRequest, TResponse>(export.Metadata.Language, (IRequestHandler<TRequest, TResponse>)export.Value))
Expand All @@ -84,9 +84,13 @@ private Task<Dictionary<string, ExportHandler<TRequest, TResponse>>> LoadExportH
.Select(plugin => new PluginExportHandler<TRequest, TResponse>(EndpointName, plugin))
.Cast<ExportHandler<TRequest, TResponse>>();

// Group handlers by language and sort each group for consistency
return Task.FromResult(interfaceHandlers
.Concat(plugins)
.ToDictionary(export => export.Language));
.Concat(plugins)
.GroupBy(export => export.Language, StringComparer.OrdinalIgnoreCase)
.ToDictionary(
group => group.Key,
group => group.OrderBy(g => g).ToArray()));
}

public string EndpointName { get; }
Expand Down Expand Up @@ -142,18 +146,75 @@ private Task<object> HandleLanguageRequest(string language, TRequest request, Re
{
if (!string.IsNullOrEmpty(language))
{
return HandleSingleRequest(language, request, packet);
return HandleRequestForLanguage(language, request, packet);
}

return HandleAllRequest(request, packet);
}

private async Task<object> HandleSingleRequest(string language, TRequest request, RequestPacket packet)
private async Task<IAggregateResponse> AggregateResponsesFromLanguageHandlers(ExportHandler<TRequest, TResponse>[] handlers, TRequest request)
{
IAggregateResponse aggregateResponse = null;

var responses = new List<Task<TResponse>>();
foreach (var handler in handlers)
dmgonch marked this conversation as resolved.
Show resolved Hide resolved
{
responses.Add(handler.Handle(request));
}

foreach (IAggregateResponse response in await Task.WhenAll(responses))
{
if (aggregateResponse != null)
{
aggregateResponse = aggregateResponse.Merge(response);
}
else
{
aggregateResponse = response;
}
}

return aggregateResponse;
}

private async Task<object> GetFirstNotEmptyResponseFromHandlers(ExportHandler<TRequest, TResponse>[] handlers, TRequest request)
{
var responses = new List<Task<TResponse>>();
foreach (var handler in handlers)
{
responses.Add(handler.Handle(request));
}

foreach (object response in await Task.WhenAll(responses))
{
var canBeEmptyResponse = response as ICanBeEmptyResponse;
if (canBeEmptyResponse != null)
{
if (!canBeEmptyResponse.IsEmpty)
{
return response;
}
}
else if (response != null)
{
return response;
}
}

return null;
}

private async Task<object> HandleRequestForLanguage(string language, TRequest request, RequestPacket packet)
{
var exports = await _exports.Value;
if (exports.TryGetValue(language, out var handler))
if (exports.TryGetValue(language, out var handlers))
{
return await handler.Handle(request);
if (_canBeAggregated)
{
return await AggregateResponsesFromLanguageHandlers(handlers, request);
}

return await GetFirstNotEmptyResponseFromHandlers(handlers, request);
}

throw new NotSupportedException($"{language} does not support {EndpointName}");
Expand All @@ -169,11 +230,10 @@ private async Task<object> HandleAllRequest(TRequest request, RequestPacket pack
var exports = await _exports.Value;

IAggregateResponse aggregateResponse = null;

var responses = new List<Task<TResponse>>();
foreach (var handler in exports.Values)
var responses = new List<Task<IAggregateResponse>>();
foreach (var export in exports)
{
responses.Add(handler.Handle(request));
responses.Add(AggregateResponsesFromLanguageHandlers(export.Value, request));
}

foreach (IAggregateResponse exportResponse in await Task.WhenAll(responses))
Expand Down
5 changes: 4 additions & 1 deletion src/OmniSharp.Host/Endpoint/Exports/ExportHandler.cs
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
using System;
using System.Threading.Tasks;

namespace OmniSharp.Endpoint.Exports
{
abstract class ExportHandler<TRequest, TResponse>
abstract class ExportHandler<TRequest, TResponse> : IComparable<ExportHandler<TRequest, TResponse>>
{
protected ExportHandler(string language)
{
Language = language;
}

public string Language { get; }

public abstract int CompareTo(ExportHandler<TRequest, TResponse> other);
public abstract Task<TResponse> Handle(TRequest request);
}
}
11 changes: 11 additions & 0 deletions src/OmniSharp.Host/Endpoint/Exports/PluginExportHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,17 @@ public PluginExportHandler(string endpoint, Plugin plugin) : base(plugin.Config.
_plugin = plugin;
}

public override int CompareTo(ExportHandler<TRequest, TResponse> other)
{
var otherPlugin = other as PluginExportHandler<TRequest, TResponse>;
if (otherPlugin == null)
{
return 1;
}

return _plugin.Key.CompareTo(otherPlugin._plugin.Key);
}

public override Task<TResponse> Handle(TRequest request)
{
return _plugin.Handle<TRequest, TResponse>(_endpoint, request);
Expand Down
11 changes: 11 additions & 0 deletions src/OmniSharp.Host/Endpoint/Exports/RequestHandlerExportHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ public RequestHandlerExportHandler(string language, IRequestHandler<TRequest, TR
_handler = handler;
}

public override int CompareTo(ExportHandler<TRequest, TResponse> other)
{
var otherHandler = other as RequestHandlerExportHandler<TRequest, TResponse>;
if (otherHandler == null)
{
return 1;
}

return _handler.GetType().ToString().CompareTo(otherHandler._handler.GetType().ToString());
}

public override Task<TResponse> Handle(TRequest request)
{
return _handler.Handle(request);
Expand Down
10 changes: 6 additions & 4 deletions src/OmniSharp.Host/Mef/MefValueProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ namespace OmniSharp.Mef
internal class MefValueProvider<T> : ExportDescriptorProvider
{
private readonly T _item;
private readonly IDictionary<string, object> _metadata;

public MefValueProvider(T item)
public MefValueProvider(T item, IDictionary<string, object> metadata)
{
_item = item;
_metadata = metadata;
}

public override IEnumerable<ExportDescriptorPromise> GetExportDescriptors(CompositionContract contract, DependencyAccessor descriptorAccessor)
Expand All @@ -19,16 +21,16 @@ public override IEnumerable<ExportDescriptorPromise> GetExportDescriptors(Compos
{
yield return new ExportDescriptorPromise(contract, string.Empty, true,
() => Enumerable.Empty<CompositionDependency>(),
deps => ExportDescriptor.Create((context, operation) => _item, new Dictionary<string, object>()));
deps => ExportDescriptor.Create((context, operation) => _item, _metadata ?? new Dictionary<string, object>()));
}
}
}

internal static class MefValueProvider
{
public static MefValueProvider<T> From<T>(T value)
public static MefValueProvider<T> From<T>(T value, IDictionary<string, object> metadata = null)
{
return new MefValueProvider<T>(value);
return new MefValueProvider<T>(value, metadata);
}
}
}
Loading