patterncsharpModerate
Multithreading class for asynchronous usage of a custom client/server exchange
Viewed 0 times
asynchronousexchangecustomusageclientforservermultithreadingclass
Problem
I've thrown everything I can at this, and I can't get it to lock or crash. My hope is that I have applied the principles correctly. I write client apps in JavaScript, and this is only the 3rd .NET class I've ever written (using JScript), so a topic as advanced as this really needs a once-over by some experienced developers.
This is a wrapper for the client class I wrote for a custom client/server exchange. The purpose is to allow asynchronous usage via threading. Originally I wanted to use the thread pool since each task shouldn't last very long, but JScript.NET cannot create delegates. The supposed solutions to this found here and here won't compile. So instead I set out to create my own threads. It's basically just a producer/consumer paradigm.
Rather than create many threads for processing calls, I opted to use a single long-lasting thread and pulse/wait to handle the calls. This is mainly because the server operates in a single thread anyway, so it can't accept multiple connections. This worked well, but on refreshing my .hta file, the class tried to reuse the same thread. Correcting that would require code to run on the unload event. In my experience that event doesn't always fire, but I'm not really sure why.
I thought it best to just use a separate thread for each consumer. I thought that went well, until I fired a test load of 500k calls and realized too late that I hadn't thought to throttle the thread generation. I fixed that by allowing each consumer to determine if another consumer is required after it has finished processing the current queue item.
I'm hoping for some feedback on if I made good choices, as well as if the output is a reasonable way to accomplish my goal.
Notes:
My comments are right-aligned, so you might not see them in the code viewport (I'm not good at reading code when it's interspersed with comments), but I have included a brief explanation of what I was trying to do, and why I did it at the head of each function.
This is a wrapper for the client class I wrote for a custom client/server exchange. The purpose is to allow asynchronous usage via threading. Originally I wanted to use the thread pool since each task shouldn't last very long, but JScript.NET cannot create delegates. The supposed solutions to this found here and here won't compile. So instead I set out to create my own threads. It's basically just a producer/consumer paradigm.
Rather than create many threads for processing calls, I opted to use a single long-lasting thread and pulse/wait to handle the calls. This is mainly because the server operates in a single thread anyway, so it can't accept multiple connections. This worked well, but on refreshing my .hta file, the class tried to reuse the same thread. Correcting that would require code to run on the unload event. In my experience that event doesn't always fire, but I'm not really sure why.
I thought it best to just use a separate thread for each consumer. I thought that went well, until I fired a test load of 500k calls and realized too late that I hadn't thought to throttle the thread generation. I fixed that by allowing each consumer to determine if another consumer is required after it has finished processing the current queue item.
I'm hoping for some feedback on if I made good choices, as well as if the output is a reasonable way to accomplish my goal.
Notes:
My comments are right-aligned, so you might not see them in the code viewport (I'm not good at reading code when it's interspersed with comments), but I have included a brief explanation of what I was trying to do, and why I did it at the head of each function.
dataLock
Solution
Don't right align your comments that way. It makes it near impossible to read the code and the comment at the same time. They're useless out there. I personally don't mind end of line comments, but right aligned... no. Others may disagree with me though.
To clarify, this is hard to read:
This is easier to read:
You seem to use whitespace inconsistently.
Personally, I would remove some of it. You don't have to, but be consistent.
There's also an empty Else statement. What happens to your function if it follows this path? If it's important to take an action, take that action; otherwise, remove it.
In
There is a 32 line comment just inside your
To clarify, this is hard to read:
delete receivedMessage[id]; // we are finished with this information.This is easier to read:
delete receivedMessage[id]; // we are finished with this information.You seem to use whitespace inconsistently.
if (Monitor.IsEntered(connectionLock)) {
try {
connected = isConnected;
}
finally {
Monitor.Exit(connectionLock);
}
}Personally, I would remove some of it. You don't have to, but be consistent.
queryMessage seems a little ambiguous. Consider how you could give it a better name.There's also an empty Else statement. What happens to your function if it follows this path? If it's important to take an action, take that action; otherwise, remove it.
In
connectThread you have some hardcoded values. Consider using constants.There is a 32 line comment just inside your
consumeMessage function. If you need 32 lines of comment to explain yourself, there's a smell and you're doing something wrong. Can you more simply explain yourself, or is there actually a smell?Code Snippets
delete receivedMessage[id]; // we are finished with this information.delete receivedMessage[id]; // we are finished with this information.if (Monitor.IsEntered(connectionLock)) {
try {
connected = isConnected;
}
finally {
Monitor.Exit(connectionLock);
}
}Context
StackExchange Code Review Q#52124, answer score: 10
Revisions (0)
No revisions yet.