Skip to content

Commit

Permalink
Fix a couple of semi-edge-cases (#199)
Browse files Browse the repository at this point in the history
* Avoid throwing on IDisposable

* Avoid throwing in ProtoBufMarshallerFactory.CanSerialize

There is at least one situation where the underlying protobuf TypeModel
throws a NotSupportedException : when asking about an array of arrays.
While this seems doubtful (it should simply return false), it is easy
enough to prevent this from trashing the whole service.

* Remove remnant of old code

No longer any reason to handle delegates specifically : it will be done
naturally by the marshallerCache (and who knows, maybe it *can*
serialize some delegates !).
Moreover, this could misinterpret a signature as valid without parameter,
with somewhat weird consequences.

---------

Co-authored-by: Marc Gravell <[email protected]>
  • Loading branch information
lanfeust69 and mgravell authored Feb 2, 2023
1 parent fc2761c commit 4781096
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 9 deletions.
18 changes: 14 additions & 4 deletions src/protobuf-net.Grpc/Configuration/ProtoBufMarshallerFactory.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using ProtoBuf.Meta;
using ProtoBuf.Meta;
using System;
using System.Buffers;
using System.IO;
Expand Down Expand Up @@ -187,9 +187,19 @@ private T ContextualDeserialize<T>(global::Grpc.Core.DeserializationContext cont
/// Indicates whether a type should be considered as a serializable data type
/// </summary>
protected internal override bool CanSerialize(Type type)
=> HasSingle(Options.ContractTypesOnly)
? _model.CanSerializeContractType(type)
: _model.CanSerialize(type);
{
try
{
return HasSingle(Options.ContractTypesOnly)
? _model.CanSerializeContractType(type)
: _model.CanSerialize(type);
}
catch (NotSupportedException)
{
// a typical case is the use of jagged arrays
return false;
}
}

/// <summary>
/// Deserializes an object from a payload
Expand Down
4 changes: 1 addition & 3 deletions src/protobuf-net.Grpc/Internal/ContractOperation.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Reflection;
using Grpc.Core;
Expand Down Expand Up @@ -212,8 +212,6 @@ static TypeCategory GetCategory(MarshallerCache marshallerCache, Type type, IBin
if (genType == typeof(AsyncServerStreamingCall<>)) return TypeCategory.AsyncServerStreamingCall;
}

if (typeof(Delegate).IsAssignableFrom(type)) return TypeCategory.None; // yeah, that's not going to happen

if (marshallerCache.CanSerializeType(type)) return TypeCategory.Data;
bindContext?.LogWarning("Type cannot be serialized; ignoring: {0}", type.FullName);
return TypeCategory.Invalid;
Expand Down
9 changes: 7 additions & 2 deletions src/protobuf-net.Grpc/Internal/ProxyEmitter.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Grpc.Core;
using Grpc.Core;
using ProtoBuf.Grpc.Client;
using ProtoBuf.Grpc.Configuration;
using System;
Expand Down Expand Up @@ -212,7 +212,12 @@ FieldBuilder Marshaller(Type forType)
var shallMethodBeImplemented = isService || isMethodInherited;
if (!(shallMethodBeImplemented && ContractOperation.TryIdentifySignature(iMethod, binderConfig, out var op, null)))
{
il.ThrowException(typeof(NotSupportedException));
// it is frequent for some infrastructure code to always call Dispose() on IDisposable,
// for instance Asp.Net Core dependency injection, so we don't want to throw in this case
if (iType == typeof(IDisposable) && iMethod.Name == nameof(IDisposable.Dispose))
il.Emit(OpCodes.Ret);
else
il.ThrowException(typeof(NotSupportedException));
continue;
}

Expand Down

0 comments on commit 4781096

Please sign in to comment.