Skip to content

Conversation

tsimbalar
Copy link
Member

What issue does this PR address?
Get rid of errors and warnings reported by ReSharper Solution-wide analysis

Does this PR introduce a breaking change?
Nope

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • N/A Unit Tests for the changes have been added (for bug fixes / features)

Other information:
Before :
before-resharper-cleanup

After :
after-resharper-cleanup

There are still some warnings; but I was not sure how to fix them :

Solution Serilog.sln
    Project Serilog
      src\Serilog\Capturing\PropertyValueConverter.cs:250 Possible 'System.NullReferenceException'
      src\Serilog\Capturing\PropertyValueConverter.cs:250 Possible 'System.NullReferenceException'
      src\Serilog\Context\LogContext.cs:257 Possible 'System.NullReferenceException'
      src\Serilog\Core\Pipeline\MessageTemplateCache.cs:54 The field is sometimes used inside synchronized block and sometimes used without synchronization
      src\Serilog\Core\Pipeline\MessageTemplateCache.cs:54 The field is sometimes used inside synchronized block and sometimes used without synchronization
      src\Serilog\Core\Pipeline\MessageTemplateCache.cs:54 The field is sometimes used inside synchronized block and sometimes used without synchronization

@nblumhardt
Copy link
Member

Great! Will have a look in detail ASAP.

      src\Serilog\Capturing\PropertyValueConverter.cs:250 Possible 'System.NullReferenceException'

This is a false positive, can be ignored.

      src\Serilog\Context\LogContext.cs:257 Possible 'System.NullReferenceException'

I am not 100% confident this can never occur, could ignore with a comment to that effect.

      src\Serilog\Core\Pipeline\MessageTemplateCache.cs:54 The field is sometimes used inside synchronized block and sometimes used without synchronization

This is by design and can be ignored.

@tsimbalar
Copy link
Member Author

Thanks, I've updated with your comments :)

ReSharper status :
image

image

Out of curiosity, I looked up ObjectHandle, the base class of DisposableObjectHandle used in LogContext, and the reference source contains :

public override Object InitializeLifetimeService()
{
    BCLDebug.Trace("REMOTE", "ObjectHandle.InitializeLifetimeService");

    //
    // If the wrapped object has implemented InitializeLifetimeService to return null,
    // we don't want to go to the base class (which will result in a lease being
    // requested from the MarshalByRefObject, which starts up the LeaseManager,
    // which starts up the ThreadPool, adding three threads to the process.
    // We check if the wrapped object is a MarshalByRef object, and call InitializeLifetimeServices on it
    // and if it returns null, we return null. Otherwise we fall back to the old behavior.
    //

    MarshalByRefObject mbr = WrappedObject as MarshalByRefObject;
    if (mbr != null) {
        Object o = mbr.InitializeLifetimeService();
        if (o == null)
            return null;
    }
    ILease lease = (ILease)base.InitializeLifetimeService();
    return lease;
}

.... so it may indeed return null.

Maybe

public override object InitializeLifetimeService()
{
var lease = (ILease)base.InitializeLifetimeService();
lease.Register(LifeTimeSponsor);
return lease;
}

could be changed to become:

public override object InitializeLifetimeService()
{
    var lease = base.InitializeLifetimeService() as ILease;
    lease?.Register(LifeTimeSponsor);
    return lease;
}

@nblumhardt
Copy link
Member

That would be a fair change. I don't think we ever create a handle to a MarshalByRefObject here, but the clarity would be improved anyway.

Will merge as-is so that we don't drift into merge-conflict territory in the meantime :-)

@nblumhardt nblumhardt merged commit c10e625 into serilog:dev Oct 19, 2017
@tsimbalar tsimbalar deleted the resharper-green-again branch October 19, 2017 06:58
@nblumhardt nblumhardt mentioned this pull request Nov 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants