patterncsharpMinor
Receiving MSMQ messages as a list
Viewed 0 times
listmessagesmsmqreceiving
Problem
I am unsure if the list could store a large number of messages.
There are 50,000 queue messages which I am receiving and assigning to the messages list:
Is this the correct way, or is there an alternative? Let me hear your suggestions.
There are 50,000 queue messages which I am receiving and assigning to the messages list:
var msgEnumerator = msgQueue.GetMessageEnumerator2();
var messages = new List();
while(msgEnumerator.MoveNext(new TimeSpan(0, 0, 1))) {
var msg = msgQueue.ReceiveById(msgEnumerator.Current.Id, new TimeSpan(0, 0, 1));
messages.Add(msg);
}
foreach( var k in messages){
MailMessage mailM = (k.Body as SerializeableMailMessage).GetMailMessage();
try {
SmtpClient sp = new SmtpClient(smtpip, 25);
sp.EnableSsl = false;
sp.Send(mailM);
}
catch (Exception ex){
logger.ErrorException("General error sending email.", ex);
}
}Is this the correct way, or is there an alternative? Let me hear your suggestions.
Solution
- You create a lot of
TimeSpanobjects which is unnecessary. Just create onetimeoutvariable. Especially since it's used in multiple places. This makes changing the timeout later easier. Otherwise you are bound to forget one place.
- Prefer
TimeSpan.FromXYZ()methods. They makes it more obvious for what period you create aTimeSpan.
- Consider moving the mail sending code into its own function which just accepts a single message and sends it.
-
It might make sense to iterate over the message queue directly instead of buffering them all up:
var timeout = TimeSpan.FromSeconds(1);
var queueIter = msgQueue.GetMessageEnumerator2();
while (queueIter.MoveNext(timeout))
{
using (var message = msgQueue.ReceiveById(queueIter.Current.Id, timeout))
{
SendMessage(message);
}
}- Instead of directly sending the message consider making an event available on the class like
MessageReceived. Then other code can subscribe to it and do stuff (like sending it as mail or logging it or saving it somewhere, etc).
- As mentioned in the comment you should wrap all objects you create which are
IDisposableintousingstatements to make sure any allocated resources are cleaned up properly.
Code Snippets
var timeout = TimeSpan.FromSeconds(1);
var queueIter = msgQueue.GetMessageEnumerator2();
while (queueIter.MoveNext(timeout))
{
using (var message = msgQueue.ReceiveById(queueIter.Current.Id, timeout))
{
SendMessage(message);
}
}Context
StackExchange Code Review Q#33121, answer score: 4
Revisions (0)
No revisions yet.