Что вы думаете о моей реализации шаблона IDisposable?

Question

Что вы думаете о следующей IDisposable реализации шаблона?

public class Connection : IDisposable 
{
    private Socket _socket;

    public bool IsConnected()
    {
        if (_socket.Poll(1, SelectMode.SelectRead) && _socket.Available == 0)
            return false;
        return true;
    }

    public void Disconnect()
    {
        if (m_socket != null && IsConnected())
        {
            try
            {
                _socket.Shutdown(SocketShutdown.Both);
                _socket.Disconnect(false);
            }
            catch (SocketException se)
            {
                System.Console.WriteLine(se.Message);
            }
        }
    }

    ~Connection()
    {
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!IsConnected())
        {
            if (disposing)
            {
                Disconnect();
            }
            else
            {
                AppDomain currentDomain = AppDomain.CurrentDomain;
                if (currentDomain.IsFinalizingForUnload() && !Environment.HasShutdownStarted)
                {
                     System.Console.WriteLine("Client failed to call Destroy");
                }
            }
        }
    }
}

Я получил эту ошибку, используя код выше:

{"Операция была предпринята для чего-то, что не является сокетом"} System.Net.Sockets.Socket.Poll (микросекунды Int32, режим SelectMode)

Answer 1

Реализация серьезно испорчена. Вы действительно не реализуете IDisposable, и в конечном итоге вы полагаетесь на сборщик мусора для очистки ваших ресурсов, что является плохой вещью.

Кроме того, вы даже не очищаете эти ресурсы должным образом, когда GC действительно приходит в себя (он делает это правильно, но по ошибке это происходит).

Ответственность за реализацию IDisposable лежит на вашем классе, поскольку вы держите ссылки, которые реализуют IDisposable. Затем в вашей реализации Dispose, если вы не получаете GCed (это явный вызов Dispose), вы должны вызывать Dispose в любых реализациях IDisposable, которые вы держите на.

Вы проверяете состояние соединения Socket, но это не то же самое, что вызов Dispose для него, и в результате вы теряете ресурс (GC в конечном итоге подхватывает его).

Инструкции по правильной реализации IDisposable см. В разделе документации MSDN под названием «Внедрение, завершение и утилизация для очистки неуправляемых ресурсов», расположенном здесь:

http://msdn.microsoft.com/en-us/library/b1yfkh5e(VS.71).aspx

Я должен отметить, что я не полностью согласен с этими руководящими принципами, но они являются наиболее принятыми. Для моей позиции см. Здесь:

http://www.caspershouse.com/post/A-Better-Implementation-Pattern-for-IDisposable.aspx

Answer 2

Это не указано явно в других ответах, поэтому, если вам интересно, что конкретно идет не так, вот оно.

Когда два объекта имеют финализаторы и возвращаются GC, нет определенного порядка, в котором выполняются их финализаторы. В вашем коде класс Socket имеет финализатор, а ваш класс - финализатор. Если финализатор сначала выполняется на экземпляре Socket, то при выполнении финализатора вы будете пытаться вызывать методы для объекта, который был завершен, и, следовательно, исключение.

Финализаторы в основном отстой. Вам вряд ли когда-нибудь понадобится написать один (даже если вы имеете дело с необработанными дескрипторами Win32 - используйте вместо этого SafeHandle.)

Вместо этого просто реализуйте IDisposable и не пишите финализатор.

Answer 3

Эта реализация имеет недостатки по нескольким причинам.

Во-первых, у вашего метода Dispose () должна быть одна цель - вызвать socket.Dispose();. Сейчас вы вкладываете слишком много логики, а не «избавляетесь» от единственного управляемого, IDisposable ресурса, которым вы владеете.

Во-вторых, вам вообще не нужен финализатор, поскольку вы не владеете напрямую или не выделяете какие-либо собственные неуправляемые ресурсы. Единственный ресурс, которым вы располагаете, - это Socket, который управляется и при необходимости будет реализовывать свой собственный финализатор. Если вы хотите перехватить и найти случаи, когда Соединение не было правильно расположено, я бы настроил финализатор только для отладки, чтобы предупредить об этом случае. Финализатор в IDisposable предназначен для обработки случая, когда сборщик мусора должен выполнить очистку, потому что вызывающий объект забыл вызвать Dispose () - в вашем случае финализатор сокета позаботится об этом за вас.

В-третьих, часть шаблона IDisposable, предложенного в рекомендациях Microsoft по проектированию, гласит, что клиент должен иметь возможность вызывать Dispose () несколько раз без каких-либо последствий. Не должно быть ничего, использующего сокет непосредственно после первого вызова Dispose () - на самом деле, я бы предложил, чтобы Dispose () вызвал socket.Close(); или (socket as IDisposable).Dispose(); и немедленно установил socket = null;, чтобы предотвратить это. С вашей текущей логикой очень возможно, чтобы вызовы в IsConnected() заставляли сокет генерировать исключение при последующих вызовах Dispose (), чего следует избегать.

В-четвертых, настоятельно рекомендуется использовать метод Close () для всех ресурсов, которые используют файл, сокет или другие «закрываемые» ресурсы. Close () должен вызвать Dispose ().

Наконец, должны быть проверки после утилизации для использования Соединения. Любой метод подключения, используемый после удаления, должен вызывать исключение ObjectDisposedException.

Answer 4

Для начала, почему вы не внедрили IDisposable?

Это дало бы четкое указание пользователям вашего объекта, что им действительно нужно избавиться от него, когда они будут готовы. Затем они могут также обернуть его в using блок и т. Д.

Answer 5

Финализаторы - это боль, и вам следует избегать, чтобы они назывались , называемыми , если это возможно. Однако если вы реализовали IDisposable, вы почти всегда захотите написать его (в сочетании с методом Dispose (bool)). Это вызывает Dispose (false) и GC.SuppressFinalize ().

Если вы не реализуете финализатор, то, если в каких-либо IDisposable экземплярах, которые вы правильно удерживаете, финализаторы реализуются, вы пропустите. Скажем, например, что у SafeHandle не было финализатора, а кто-то вызывал Dispose? Если вы держите его и не внедрили финализатор, то вы позволяете своему потребителю навсегда утратить дескриптор, не вызывая Dispose для вас!

Если предположить, что и ваш потребитель, и предметы, которые вы потребляете, будут играть хорошо, это не повод не играть себя хорошо.

Кроме того, даже если вы используете SafeHandle, вы должны утилизировать его, как если бы это был управляемый объект. В противном случае, если вы не утилизируете его вообще, ему придется ждать, пока GC не будет выпущен, и если вы сделаете его утилизировать в Dispose, но без финализатора, и никто не избавится от вас, тогда ему придется подождать до GC после того, который собрал вас.

b_richardson выше имеет правильный шаблон, это не сложно, и добавляя GC.SuppressFinalize (), вы избегаете двухпроходного GC, за исключением случаев, когда вы не утилизируете должным образом.

Что касается добавления логического значения для отслеживания, если мы уже были удалены, это будет работать, я предпочитаю просто написать свои методы Dispose для повторного запуска (проверка на нулевое значение и т. Д.), Также помогает сделать это таким образом когда объект может получить ресурсы во время использования, и вы хотите распоряжаться только теми, которые у него есть.

Answer 6

Я просто хочу добавить это к шаблону, указанному Ричардсоном

private bool disposed;

private void Dispose(bool disposing)
{
    if(!this.disposed)
    {
        ReleaseUnmanagedResources();
        if (disposing)
            ReleaseManagedResources();
        this.disposed = true;
    }
}

Answer 7

Это то, что я использовал в прошлом для одноразового шаблона (я бы предложил начать с этого):

public class Connection : IDisposable
{
    #region Dispose pattern

    /// <summary>
    /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
    /// </summary>
    /// <filterpriority>2</filterpriority>
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool disposing)
    {
        ReleaseUnmanagedResources();
        if (disposing)
            ReleaseManagedResources();
    }

    /// <summary>
    /// Derived classes from this class should override this method
    /// to clean up managed resources when Dispose is called.
    /// </summary>
    protected virtual void ReleaseManagedResources()
    {
        // Enter managed resource cleanup code here.
    }

    /// <summary>
    /// Derived classes should override this method to clean up
    /// unmanaged resources when Dispose is called.
    /// </summary>
    protected virtual void ReleaseUnmanagedResources()
    {
        // Enter unmanaged resource cleanup code here.
    }

    #endregion