2
\$\begingroup\$

Is there anything wrong with doing this:

Public Class Form1
    Private Sub Form1_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
        For value As Integer = 0 To 1000000
            Dim p As New Person
            p.ID = value
            p.DoSomething()
            p = Nothing
        Next
    End Sub
    Public Class Person
        Dim _ID As Integer
        Public WriteOnly Property ID() As Integer
            Set(ByVal value As Integer)
                _ID = value
            End Set
        End Property

        Public Sub DoSomething()
            _ID = _ID + 1
            'Do more with ID
        End Sub
    End Class
End Class

As appose to this:

Public Class Form1
    Private Sub Form1_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
        For value As Integer = 0 To 1000000
            Person.DoSomething(value)
        Next
    End Sub
    Public Class Person
        Public Shared Sub DoSomething(ByVal id As Integer)
            id = id + 1
            'Do more with ID
        End Sub
    End Class

End Class

I have written some code similar to that in sample 1. I have read online that code 1 can cause problems when threading is involved, but I do not understand why? Are there any other problems with sample 1? Sample 1 meets my requirements better.

\$\endgroup\$
3
\$\begingroup\$

In sample1, as you call it, you have an instance field which you change. Therefore, the Person class you are using is mutable. Mutable classes are prominent to concurrency issues if a single instance is manipulated by multiple threads, unless explicit synchronization mechanism is properly involved.

In sample2, if you pass the value in the method, and you are not modifying the state of the Person class as you do your business logic, then the code is thread safe. No other thread can access the local variable id and modify it concurrently, since it is visible to the current thread only. So there is no need to apply a synchronization mechanism.

However, sample2 may not be appropriate enough for your concrete requirements. If you give more code clues on what happens in the method in question, then a more-accurate advice can be given.

\$\endgroup\$
1
\$\begingroup\$

The reason that the first example would be considered problematic when multiple threads are involved, is because in that example, a Person object maintains state (i.e. data) and it has methods that act upon that state. There's nothing wrong with designing classes that way, per se, and it doesn't mean that the class cannot be used by multi-threaded applications safely--it just makes multi-threading more difficult.

For instance, consider this example:

Public Class Form1
    Dim _p As New Person

    Private Sub DoWork(value As Integer)
        _p.Id = value
        _p.DoSomething()
    End Sub

    ' ... Code that calls DoWork on multiple threads ...

    Public Class Person
        Dim _id As Integer
        Public WriteOnly Property Id() As Integer
            Set(ByVal value As Integer)
                _id = value
            End Set
        End Property

        Public Sub DoSomething()
            _id = _id + 1
            'Do more with Id
        End Sub
    End Class
End Class

Let's say that in the above example, the DoWork method is called from two different threads simultaneously, and the order that the lines are processed is like this:

  1. Thread A sets _p.Id = 1
  2. Thread B sets _p.Id = 10
  3. Thread A calls _p.DoSomething which increments _p.Id to 11
  4. Thread B calls _p.DoSomething which increments _p.Id to 12

As you can see, that would certainly be undesired behavior. There are three ways to fix this problem. The first way is to use one of the available synchronization techniques to make sure that only one thread uses the object at a time, for instance:

Private Sub DoWork(value As Integer)
    SyncLock _p
        _p.Id = value
        _p.DoSomething()
    End SyncLock
End Sub

By synchronously locking on the object, if a second thread tries to call DoWork at the same time, the second thread's execution will be blocked (i.e. hung) until the first thread's call to DoWork is complete. The problem with this approach is that, at least to some extent, it defeats the purpose of multi-threading because only one thread can do that part of the work at a time.

The second way to fix this would be to have each thread create a new Person object so that they don't interfere with each other. For instance, instead of having the _p object declared as a class-level field, you could just create a new Person object inside the DoWork method, like this:

Private Sub DoWork(value As Integer)
    Dim p As New Person()
    p.Id = value
    p.DoSomething()
End Sub

The third way to fix this would be to use your second example. By fixing the Person class so that it is a stateless class, multiple threads can call the same object as often at the same time as necessary because the data is stored outside of the Person class. So, as long as each thread is instantiating a separate object to store the data before passing it into the Person object, all the threads can share the same Person object without needing to worry about collisions.

\$\endgroup\$
  • \$\begingroup\$ Thanks. +1. I assume that if I do not create any Threads myself i.e. using the Thread class, then there is no issues? \$\endgroup\$ – w0051977 Apr 9 '13 at 21:15
  • 1
    \$\begingroup\$ It's not a matter of how you create your threads--it's a matter, rather, of how the threads use your Person class. If multiple threads share the same Person object, then you should have some concerns. If they each use their own independent Person object, then it's not a concern (unless the Person class contains shared members or shares dependencies between instances). \$\endgroup\$ – Steven Doggart Apr 10 '13 at 11:41
  • \$\begingroup\$ Thanks for the helpful comment. I assume that if I do not explicitly create any Threads then there is no issue to even think about? The reason I ask is because I know that ASP.NET uses MTA's (multi threaded apartments). \$\endgroup\$ – w0051977 Apr 10 '13 at 16:16
  • \$\begingroup\$ Correct. If you are not explicitly making your code multi-threaded, none of it is an immediate concern. \$\endgroup\$ – Steven Doggart Apr 10 '13 at 16:19
  • \$\begingroup\$ Thanks I assume that the fact that ASP.NET uses MTA is insignificant. I know what MTA's are in theory but I do not understand the relationship between MTAs and threads (System.Thread). Thanks again. \$\endgroup\$ – w0051977 Apr 10 '13 at 16:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.