|
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!
|
|
|
|
|
Thanks but if there is no difference, I will leave it with deep copy, since I already implemented that.
|
|
|
|
|
1. Sounds good
2. It's pretty much another way to deep copy. But also, if you create a new struct, you can decide what your client has access to - at this point, it has access to all members of SocketStructure. With a struct, you would decide exactly what your client sees.
It the case of a simple structure (such as your SocketStructure), having a struct would probably be overkill, but for more complex classes, it might be a solution.
Best,
John
|
|
|
|
|
2. It's pretty much another way to deep copy. But also, if you create a new struct, you can decide what your client has access to - at this point, it has access to all members of SocketStructure. With a struct, you would decide exactly what your client sees.
Not sure I understood that - maybe because I am not very experienced with .NET yet. But if from thread safety point of view there is no difference whether I do a deep copy like I showed in my code, or return copied structs, I will stick to my approach then ...
It is ok if client has access to all members ...
|
|
|
|
|
Member 12061600 wrote: I will stick to my approach then ...
Sounds good
Best,
John
|
|
|
|
|
|
You're welcome!
Best,
John
-- LogWizard Meet the Log Viewer that makes monitoring log files a joy!
|
|
|
|
|
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.
Then every time I want to access the hostAddresses list I should use a getter instead of storing it in a a tmp variable like I have above, isn't it? because modifying (adding/removing items from) the hostList is protected with a lock inside say Add function, and if someone modifies it between two executions of getters, that is fine, I can't control that, but if I used tmp variable - then each time it would show old values
|
|
|
|
|
If SocketStructure is a struct or an immutable class, then you might want to consider using the immutable collections[^] from NuGet[^].
That way, any modifications to the collection will create a new collection instance, and replace the existing field. Any threads which have already retrieved a version of the collection will not see the changes unless they retrieve it again.
The immutable collections were designed to minimize the cost of creating a new version of the collection for every change.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
