|
You may not like the previous answer, but it is still correct. Hence, I upvoted it.
Try a matlab forum, or show us some C# code.
Bastard Programmer from Hell 
If you can't read my code, try converting it here[^]
|
|
|
|
|
Imagine I have such code:
public static List<SocketStructure> GetHostAddressesList()
{
lock (m_locker)
{
if (m_isInited == false)
{
return null;
}
return m_HostAddresses;
}
}
Usage in other class
List<SocketStructure> tmp = DppModuleParameters.GetHostAddressesList();
for (int i = indexHost; i < tmp.Count; i++)
{
}
My question is you can see above once I got reference to m_HostAddresses, even though there is lock in the getter, I am losing the thread safety, because, tmp still points to m_HostAddresses right? So if another thread modifies m_HostAddresses, then tmp will have old value right?
What is the way to protect against this?
Would it be better instead of getter to have something like deep copy? I pass some temporary structure to function and instead of returning reference as getter does, the function would copy host addresses to that structure inside lock. Would this be better?
Or I should use getter each time I want to refer to that variable instead of storing in tmp?
BTw. The only method that can modify host addresses is also protected with same lock.
|
|
|
|
|
Well...yes and no.
When you set tmp to the collection, it will maintain the same value - it "points" at the same collection until you specifically set tmp to a different value with the assignment operator. But the collection it refers to can be changed by Add and Remove operations without affecting the value in the variable - it's the same collection regardless of what happens to it's contents. There is no way to "guard against this" because it is exactly what you expect to happen.
Consider your car: it's the same car if it just has you driving as it is if you add two passengers in the rear seats, or take out the spare tyre and leave it at home. The variable "my car" doesn;t change, but it's content does.
It you want thread safety with a collection, then you need to use a lock round each access to the collection: Add, Remove, Insert, and get element, not so much in the getter - unless your getter returns a copy of the collection instead of the "real thing".
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
|
|
|
|
|
With tmp variable is a bit tricky, even if I have lock in Add function,
if I store the reference in tmp it will still refer to old variable
but If I call getter each time, then it will give me new value, isn't it?
|
|
|
|
|
Member 12061600 wrote: The method which can modify my list is actually protected under SAME lock.
So I guess I am on the safe side
No, because your existing lock doesn't protect anything - you can still be accessing the list from one thread while a different thread is deleting items. You need to apply the lock each time you access or modify the content (which means pretty much every time you do tmp.anything)
A copy is safer:
return m_HostAddresses.ToList(); would do it.
But even then, if the actual items in the collection are being changed, then they need a lock as well:
SocketStructure ss = tmp[10];
ss.FavouriteFruit = new Banana(); is still not thread safe if anyone else can be accessing the same element of the original collection.
Member 12061600 wrote: but If I call getter each time, then it will give me new value, isn't it?
Just existing getter will return the same value - because it returns a reference to the collection.
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
|
|
|
|
|
Please don't delete questions, it makes life a PITA at times, if you have just typed up a reply and the question has vanished... 
You need to brush up on what a reference is: Using struct and class - what's that all about?[^] may help as it explains why a value and a reference are different.
Your getter always returns a reference: and that's true if it returns a copy or the original collection. If your getter returns the original, then you don't get a new value each time, you get the same reference - which "points at" the same collection.
If we go back to cars, then if you put your mobile in the glove box of "your car" then under normal circumstances you wouldn't expect to find it in "my car". But... what if we were married, and had a single car between us? Then "your car" and "my car" refer to the same actual vehicle, so you can put the mobile in your glove box, and I can find it in mine!
Your existing getter is returning the same "vehicle" each time it's called, so anything that one thread does to the underlying collection affects the data a different thread accesses. This is why locks are so important (and difficult to understand!)
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
|
|
|
|
|
I meant different - hard to explain.
With my existing getter, if I store reference in TMP.
And my Setter (which is locked) modifies list in another function,
tmp will still have old value?
If instead of tmp I use getter each time, I will be able to
see the changed value?
PS I am thinking to go with solution of returning copy, that seems better
|
|
|
|
|
That's where it gets complicated...
The value in tmp will only change when you use an assignment operator.
But the object it refers to can be changed!
Let's try an example.
int[] arr = new int[] {1, 2, 3, 4, 5};
int[] tmp = arr;
Both tmp and arr refer to the same array of 5 integers:
private void PrintThem(int[] data)
{
foreach (int i in data)
{
Console.WriteLine(i);
}
}
...
int[] arr = new int[] {1, 2, 3, 4, 5};
int[] tmp = arr;
PrintThem(arr);
PrintThem(tmp);
will produce the same numbers.
And if I do this:
int[] arr = new int[] {1, 2, 3, 4, 5};
int[] tmp = arr;
arr[2] = 666;
PrintThem(arr);
PrintThem(tmp);
You will get the same sequence printed twice:
1
2
666
4
5
1
2
666
4
5
It's only when I create a new array that it changes:
int[] arr = new int[] {1, 2, 3, 4, 5};
int[] tmp = arr;
arr[2] = 666;
arr = new int {7, 8, 9};
PrintThem(arr);
PrintThem(tmp);
Will produce:
7
8
9
1
2
666
4
5 If your setter modifies m_HostAddresses by assigning it a new collection:
m_HostAddresses = new List<SocketStructure>(); then there is no threading problem because the collections are different.
But if it modifies the content instead:
lock (m_locker)
{
m_HostAddresses.Add(new SocketStructure());
}
Then that isn't thread safe unless every time you access the collection you add the lock round that code as well.
Even returning a copy may not cure this problem, if any of the SocketStructure elements in the collection are altered at any point, because the copy of the collection does not generate a copy of all the elements in it, just the references to them!
This can take a bit of getting your head round, and without being able to see when your eyes start to glaze over it's very difficult to know if you understand what I mean.
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
|
|
|
|
|
I see
Everytime I modify m_HostAddresses - there is lock inside that function
Maybe I need something like this?
public static SocketStructure[] GetHostAddressesList() {
lock(m_locker)
{
List<SocketStructure>tmp = new List<SocketStructure>tmp();
for(int i = 0; i<m_HostAddress.Count(); i++)
{
SocketStructure s = new SocketStructure();
s.IP = m_HostAddress[i].IP;
s.port = m_HostAddress[i].port;
tmp.Add(s);
}
return tmp.ToList();
}
}
|
|
|
|
|
That's still "complicated" - because you may need the lock because of the m_HostAddress collection!
The List now doesn't need it because it can't be accessed outside the method: it's created inside the method, and the only reference to it - tmp - goes out of scope at the end. So technically, if you didn't have the Host Address collection involved, you wouldn't need a lock at all.
And your method doesn't have to return a copy at all, because the collection it returns is only referenced in the one place, and contains entirely new objects. It is a new object that is returned each time.
And to answer your other question: "IP is string type - but am I copying reference here again????" Oh yes. Strings are Reference types (but slightly special in that they are immutable) so every time you assign a string value you assign a reference to the actual string instance.
port is an integer - which is a Value type - so every time you assign an integer you are copying the value.
If you weren't, then things woudl get nasty quickly:
int a = 6;
int b = a;
b = b * 2; If integers were reference types then a and b would both contain 12!
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
|
|
|
|
|
confused 
Let's say this is my getter
public static SocketStructure[] GetHostAddressesList() {
lock(m_locker)
{
List<SocketStructure>tmp = new List<SocketStructure>tmp();
for(int i = 0; i<m_HostAddress.Count(); i++)
{
SocketStructure s = new SocketStructure();
s.IP = m_HostAddress[i].IP;
s.port = m_HostAddress[i].port;
tmp.Add(s);
}
return tmp.ToList();
}
}
And say the code that can add elements to my m_HostAddresses is inside ONE and only one function which inside locks on the same object (of course adding code is inside lock).
Am I fine given such scenario?
|
|
|
|
|
Yes, you should be - but you still don't need to return a copy of the collection becuase it's created anew each time the getter is called.
return tmp; will do fine.
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
|
|
|
|
|
If you want to protect your underlying list from being modified, then you need to return a copy of the list rather than the list itself. If you change your GetHostAddressesList slightly, you'll find that you are achieving a more effective lock:
public static SocketStructure[] GetHostAddressesList()
{
lock (m_locker)
{
if (!m_isInited) return null;
return m_HostAddresses.ToArray();
}
}
|
|
|
|
|
This is complicated in many many levels 
First off, if you want a read-only getter, and the list you return is small, then yeah, a copy would partially work:
public static List<SocketStructure> GetHostAddressesList() {
lock(m_locker) return m_HostAddress.ToList();
}
However, if SocketStructure is a class, you're still not completely safe.
Because you will have a copy of the list, but the elements themselves still point to your internal list - and they can still change as you access them.
What you want to return is a copy of the list (as shown above), and:
1. either have SocketStructure immutable
2. or make SocketStructure thread-safe as well.
3. make SocketStructure a struct (in that case each element in the returned list will be a copy of the original element)
Best,
John
|
|
|
|
|
Something like this maybe?
public static SocketStructure[] GetHostAddressesList() {
lock(m_locker)
{
List<SocketStructure>tmp = new List<SocketStructure>tmp();
for(int i = 0; i<m_HostAddress.Count(); i++)
{
SocketStructure s = new SocketStructure();
s.IP = m_HostAddress[i].IP;
s.port = m_HostAddress[i].port;
tmp.Add(s);
}
return tmp.ToList();
}
}
|
|
|
|
|
Almost Note that string is immutable, so you're fine there.
public static List<SocketStructure> GetHostAddressesList() {
lock(m_locker)
{
List<SocketStructure>tmp = new List<SocketStructure>();
for(int i = 0; i<m_HostAddress.Count(); i++)
{
SocketStructure s = new SocketStructure();
s.IP = m_HostAddress[i].IP;
s.port = m_HostAddress[i].port;
tmp.Add(s);
}
return tmp;
}
}
At the last line, you don't need to use .ToList(), since you've already created a new list.
Best,
John
|
|
|
|
|
Ok, so you recommend I use this one? Btw tmp isn't array type above
|
|
|
|
|
Yes, I do. About array - yes you're right. Just modified it now.
|
|
|
|
|
So when I am returning value types - it is fine? (from the point of view of multithreading). And assuming ALL my getters (both for value and reference types) have locks.
But if I return reference type I better make a deep copy, that is better practice you say right?
|
|
|
|
|
You nailed it
Best,
John
|
|
|
|
|
Thanks - I hope we understood each other
PS. (Same applies for strings I guess as it does with ints, since string are immutable)
PPS Just started thinking indeed returning reference to object even if you have locks inside getters is rather tricky - because some other thread might change contents of the reference type, and if you are left with the reference which you retrieved above you will see different results depending how other thread modifies it
|
|
|
|
|
Member 12061600 wrote: PS. (Same applies for strings I guess as it does with ints, since string are immutable)
Yes
Member 12061600 wrote: PPS Just started thinking indeed returning reference to object even if you have locks inside getters is rather tricky - because some other thread might change contents of the reference type, and if you are left with the reference which you retrieved above you will see different results depending how other thread modifies it
Yes, it's very tricky indeed. So I would suggest you go with the easy solution for now (immutable objects or structs)
Best,
John
-- LogWizard - Meet the Log Viewer that makes monitoring log files a joy!
|
|
|
|
|
Member 12061600 wrote: So when I am returning value types - it is fine?
Yes
Member 12061600 wrote: But if I return reference type I better make a deep copy, that is better practice you say right?
It all depends on what you're trying to achieve, but if you're not sure about it, it's safe to assume "yes". But as said in the other answer, I suggest you go for immutable objects or structs.
Best,
John
-- LogWizard - Meet the Log Viewer that makes monitoring log files a joy!
|
|
|
|
|
I already went with deep copy approach I showed you above (the code). So I will leave it like that?
Because I have classes instead of structs and they aren't immutable.
|
|
|
|
|
It's up to you. You can also create a struct used just for you clients, something like:
struct socket {
string ip;
int port;
}
and return a list of that.
Best,
John
-- LogWizard - Meet the Log Viewer that makes monitoring log fies a joy!
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
