I will attach a screenie, because that's the best way to explain what happens.
As you can see, I remove the item AFTER I get the info I need out of it.
Can someone confirm that this is impossible and I must thoroughly investigate if I'm not clearing something from the list somewhere else? Because, I have. But I will go into S&D mode if you tell me that's 100% the problem. I'll just turn the app upside-down, tare it apart if I must, till I find the little effer.
Seriously, it's been bugging me from like a week now, and I'm going :gnasher:
It happens in some cases, which really makes me think that I'm clearing a record somewhere else too. The problem is probably very small, and I'm just being very very lame..
So, sorry if this is a really lame question, I just haven't been able to get my thoughts around it from like a week.. if not two.
Actually I see now that it says the QueueHolder.Count == 0 in the debug info. Can I ask, do you have a lot of timers or other threads running in your code? It looks like this might be a concurrency issue and another thread might be updating the QueueHolder between the Count > 0 check and where you index into it. The method in the screenshot is obviously not atomic and none of the .NET generic collections are thread-safe by default. Presuming that Dizplay does not modify QueueHolder, it looks like another thread must be causing the issue.
Um, I've been an utter forum ninja as I stealth updated my post before you posted. Sorry!
It looks to me like a concurrency issue caused by multiple threads updating the QueueHolder at once (most likely multiple timer threads). That's the only way I can account for the QueueHolder.Count being zero in that screenshot. I didn't notice that the first time I looked at it, sorry.
The question is then, do you have any timers or other threads that manipulate the QueueHolder variable?
Oh man.. Multithreading - not my territory. I think that when I'm starting the timer, I'm actually creating another thread, and then when I'm stopping it I'm not clearing it? So, that would mean that when I create another thread and start the timer, it starts the other threads as well?
Just restarted my lappy, so I'll take 5mins and edit the post, to tell you if this is what's really happening (to run C#). If that's the case tho, I won't be able to reply very fast, because one of my palms will need to stay on my face for a while.
Edit: Oh god! -.-
<?php public static void Start() { if (QueueProcessor.Enabled != true) { QueueProcessor.Elapsed += new ElapsedEventHandler(Processor.QueueProcessor_Elapsed); QueueProcessor.Interval = 10; QueueProcessor.Start(); QueueProcessor.Enabled = true; } } ?>
And I'm calling that each time a button is added to the queue.
Wanted to bump (now I'm getting really annoying, haha).
So, how do I check if I have already created a thread, without having a global variable, which I set to true immediately after I start the first one? Or is that the most optimized way? Multithreading is just not my thing, yet(positivity FTW!).
Edit: God, I didn't even google it before asking the question. Will do now.
Edit2: Ok, is this efficient enough, and does it actually work as you would suggest I expect it to work?
At the bottom of the elapsed event (look snippet above if needed):
<?php if (QueueHolder.Count < 1) { QueueProcessor.Stop(); QueueProcessor.Enabled = false; ---> QueueProcessor.Elapsed -= new ElapsedEventHandler(Processor.QueueProcessor_Elapsed); } ?>
Well, love or hate multithreading (um, I think everyone hates it), InSim.NET is a multithreaded library, and each timer you add spins up yet another thread. A console application using InSim.NET with a timer will result in three threads running at once, the main program thread, the InSim.NET* receive thread, plus also the timer thread.
Now if you try to update a global variable from a timer while the receive thread is trying to update it, you will run into serious and strange bugs (like this one). The solution is that you need to employ locking, which is pretty-standard for multithreaded apps, and something .NET makes pretty simple, to its credit.
You need to create some sort of object that you can lock on.
object lockObject = new object()
Whenever you manipulate QueueHolder (or any other variable which can be access from multiple threads), you call lock on that object to prevent anyone else from trying to update it at the same time. For instance.
lock (lockObject) { QueueHolder.Add(object); }
If another thread tires to access the QueueHolder it will first run into the lock, which will cause that thread to block until the lock is released. This makes it sure that only a single thread can access that resource at once, however it has the downside that every other thread will need to wait around for the first thread to finish. You need to do this at every point you update or access QueueHolder though.
The code you posted in that screenshot would become something like this (simplified and without testing)
public static void QueueProcessor_Elapsed(object source, ElapsedEventArgs e) { int[] UCIDTimes = new int[256];
lock (lockObject) { int Count = QueueHolder.Count;
for (int i = 0; i < Count; i++) { if (QueueHolder.Count > 0 && QueueHolder[0] != null && QueueHolder[0].Button != null) { int UCID = QueueHolder[0].Button.UCID; int idx = Dizplay.getUserIdxByUCID(UCID);
if (UCIDTimes[UCID] < maxPerUCID) { clsUser U = Dizplay.Users[idx];
In this instance we just wrap the whole thing in a lock, which should do for these purposes, although really you want to make your locks as small and 'finely-grained' as possible. As I say, that's simplified and without testing through, but it should give you a good idea how to progress.
For a really nice primer on threading in C# have a read of this free e-book (chapter two on synchronisation should be of special interest to InSim.NET users).
Threading is one of those things that seems so simple on the surface, until you run into your first concurrency bug and realise that it's a black pit of death you can never escape from. Oh well...
* This is the same for every InSim library ever released, it's not peculiar to InSim.NET. It shows how weird and strange these bugs can be as to how seldom this has come up. You can guarantee that they will come up eventually though, given enough time. Incidentally this is why Windows doesn't let you update controls from different threads (and why you need to marshall invokes across the dispatcher), by allowing cross-thread access they'd open themselves up to thousandths of strange bugs they'd never be able to figure out.
Edit: Another option would be to make InSim.NET appear to run single-threaded by using a SynchronizationContext, which is possible and I might write an example of how to do that later. It would make the library much less efficient though, which is why it doesn't do that by default.
If I ever visit Glasgow, I'll make sure to drop by with some beer, or whatever it is that you drink. I definitely learned a hell of a lot, in just a few minutes of reading this and the article you've sent me. And you can be sure that I'll be reading the rest of it, very soon.
Just to clarify one thing tho, because it's still very blurry in my head: When I create those threads, they stay running, right? Is the code in my last post going to kill the thread, or is it going to just keep on running? I mean... Even if 1 player is on the server, I would like to keep the thread running, but I could achieve that with a more global var. But when the server is empty, I have nobody to send buttons to - so why keep it? I'm getting a bit off the question here - I just wanted to ask - how to kill the timer's thread?
The timer gets disposed when the timer object is removed from scope, just like all .NET objects (more or less). The only thing you need to watch out for is that there are no stray event-handlers or references lurking in the background. These can cause memory leaks and other issues, but this is the same for all .NET objects, not just timers.
There are at least five different timers in .NET (from the top of my head) and each one works slightly differently. The only way to know how to use each one is to read the documentation (select the object and press F1 in VS2010).
Really the only important thing you need to know is that if there is a possibility that two threads may try to access a resource at the same time, then you will need to synchronise the access with locks. If you don't your app will work 99.9% of the time, but that last 0.1% will make you go insane.
Thanks for the clarification. I just decided to go back on that timer, because it's all working perfectly now, but indeed, I am thinking that it's leaking memory.. Well, not exactly memory, but whole event-handling references (if that's what they are) lol! I mean, nothing is suggesting that, because after all, it doesn't really have time to build that much loops, but with those new things I know (exciting, I am starting to feel like I'm finally about to stop drowning, and have a chance to swim on top of multithreading) I am definitely going to check and optimize it if needed. It was like the opening of Pandora's box whenever I took a step into multithreading till now. And I was mindeffed yesterday, because I dug a bit into it, and yeah... My head failed in multitasking, as I was failing to understand multithreading, if you allow me to put it that way. But now the basic concept makes perfect sense in my mind, yay!
And I just had the perfect idea what to do. I will be building a string, as long as the timer is running. Before I lock the whole thing, I will add "Attempt to access resource (", and if the lock is entered - I will simply add "success". After the lock, simply another ")" and a new line added. That would make a perfect report. Ha! Thanks!
But first, I'm now going to press that F1 key on the timer object.
Well, as I said in my post, leaking memory here might be an issue. You do need to 'unhook' any events which are attached to the timer before it goes out of scope. Dangling event-handlers are the number one cause of .NET memory leaks. This will be even more of an issue if your program is designed to run for weeks at a time, like a LFS server app.
Really the best practice is to try and reuse your timer objects wherever possible, however if that is not possible then you should unhook the event-handler once you are done with it, and before that object goes out of scope.
timer.Elapsed -= timer_Elapsed
This is one of the most common memory leaks in the .NET, if not the most common, so you are correct to raise this as an issue.
Aside of the multithreading topic, there is something else that raised a brow. Although I don't think this is the cause of the out of bounds exception.
But have a read of the comments in the code :
int[] UCIDTimes = new int[256]; int Count = QueueHolder.Count;
// Loop through ALL QueueHolder entries for (int i = 0; i < Count; i++) { // It ONLY checks the first entry, always, even if i > 0 if (QueueHolder.Count > 0 && QueueHolder[0] != null && QueueHolder[0].Button != null) { // Again here it will always only work with the first entry int UCID = QueueHolder[0].Button.UCID; int idx = Dizplay.getUserIdxByUCID(UCID);
// Here it checks the UCIDTimes only for the UCID of the first entry in the queue. // So if there are ever multiple entries in the queue, but the following conditional is false for entry 0, // then this whole method gets buggy when i > 0 but you're always working with index 0. if (UCIDTimes[UCID] < maxPerUCID) { clsUser U = Dizplay.Users[idx];
// Here you again remove only the first object, even if i > 0. I don't know if this // is the cause of your problem, but it certainly invites problems. QueueHolder.RemoveAt(0); } } }
Why not replace the 0 index used everywhere with i? (but remember to do i--; after you remove an entry)
And for the rest I'd say debug the heck out of it - print the contents of the array and other values to the debug window on every turn and study them to see what happens.
And try DT's lock suggestion. If you add / remove objects in an array, and it's possible that this happens in multiple threads, you'll need that lock method quite often, so best get used to it
I think I got it? But, I am still considering myself new at this part of C#, so I'm leaving it like that, for now, until all these things cool down and take their right places in my brain. I usually just get those (as morpha calls them) "click" moments, where I just instantly have to go and do what my brain has told me, and in most cases, it usually works. It mostly happens while I'm doing something totally unrelated.
Also, Victor, thanks for the 0-to-i suggestion. I switched to that, but, really, it's the part of my code that had no problems, and I feel like the i-- part is just an empty turnover. IMO, the less code the better, and I had experimented a lot, so I saw, that when you remove the 0 index, the whole list just drops all indexes down, so I have another 0 index. And being that way, I just have it perfect, because it picks the oldest button added. Yet, I am saying all this, because I might be wrong, or this might be totally bad practice, so feel more than free to correct me if I'm wrong.
Okay, useless facts aside, I'm just going to throw the code in, so you can take advantage of it, and/or tell me if that's not the way it should be done.
<?php public static List<clsQueue> QueueHolder = new List<clsQueue>(); public static Timer QueueProcessor = new Timer(); public static int maxPerUCID = 4; public static object QP_Lock = new object(); public static string MT_Report = "";
public static void Start() { if (QueueProcessor.Enabled != true) { QueueProcessor = new Timer(); QueueProcessor.Elapsed += new ElapsedEventHandler(Processor.QueueProcessor_Elapsed); QueueProcessor.Interval = 10; QueueProcessor.Start(); QueueProcessor.Enabled = true; } }
public static void QueueProcessor_Elapsed(object source, ElapsedEventArgs e) { try { MT_Report += "Attempt to access resource ("; lock (QP_Lock) { int[] UCIDTimes = new int[256]; int Count = QueueHolder.Count; int MT_Report_i = 0;
for (int i = 0; i < QueueHolder.Count; i++) { if (QueueHolder.Count > 0 && QueueHolder[i] != null && QueueHolder[i].Button != null) { int UCID = QueueHolder[i].Button.UCID; int idx = Dizplay.getUserIdxByUCID(UCID);
if (UCIDTimes[UCID] < maxPerUCID) { clsUser U = Dizplay.Users[idx];
Sorry for being so commentless on this part of the code, where I should have the most explanations. It's just messing with my brain enough already, that explaining it would be kind of hard. But I will do it, eventually.
