Been meaning to dig into the NServiceBus code a bit and check it out
(been too busy but aware of the buzz for a while now). The code just
doesn't seem production quality to me :(

Sorry Udi. I still think you have a lot of good stuff to say though :)

Here's my thoughts from looking through the Msmq transport and worker
thread classes tonight:

MsmqTransport.cs

@ErrorQueue : set
- errorqueuepath is set before setting / constructing method queue.
Ff an exception is thrown, this object's state will be inconsistent
(easy to repro, just pass null to the setter). Recommend try / catch
or set after construction... maybe the queue should also be tested to
make sure it is valid, so we don't end up with an error queue that
doesn't work accidently?

@PurgeOnStartup : set
- what if someone calls this after startup? I lose all my messages.
Recommend throwing exception if already started up and / or actually
performing the purge on startup. Setters with side effects are
generally bad.


@NumberOfWorkerThreads
- set followed by get does not return the value set as would be
expected. Count of worker threads list is increased on start, but
setter sets a private variable. If the count is "virtual" make it
virtual. If it isn't increase the thread count before returning.
Either way, make it consistent.

@ChangeNumberOfWorkerThreads
- NumberOfWorkerThreads setter is too semantically close to this.
Recommend getting rid of one of these to avoid confusion. I
understand you can tell the difference by looking at the code, but
you shouldn't have to view source to figure this out.

- What if this is called before Start()? Threads will start
prematurely. Recommend exception if called before start.

@Start
- Why not call ChangeNumberOfWorkerThreads instead of manually
starting all the threads?

@ReceiveMessageLater
- This just calls send in both http and msmq bindings. I could just
call send again if I really just wanted to resend it... could this
also result in duplicat copies of the same message if I thought it
meant to recieve later and the message was not unqueued from the
source queue. Recommend removing or making it do something useful. At
the very least, make sure the message isn't still in the queue.
- Should response queue always be set? What if this is a one way
message?

@AddWorkerThread
- Stopped delegate locks on same lock as AddWorkerThread.
Since Stopped is called on the

@NumberOfWorkerThreads : get
- is this lock needed? Seems pointless.

@MessageTypesToBeReceived : set
- GetExtraTypes removes types from list. Why not throw an exception
if there are extra types instead so I don't get a silent failure that
is hard to track down?


@CantSerializeType
- Negative in name is bad, forces double negatives (!CantSerialize)
- Why just support for Array... consider IEnumerable<T> instead?


@Receive/ReceiveFromQueue
- Why create needless TransactionalWrappers each call? Recommend use
of a singleton or make it a static method (lambdas are nice).
- What in the world is needToAbort for? Looks like it just causes an
exception that never gets handled.... ah, I see, it is to abort the
transaction... there must be a cleaner way to do this... could we
just abort the ambient transaction or refactor and combine with
Receive?
- failuresPerMessage not locked despite comment saying "lock before
use" (line 376)
- catch of MessageQueueException swallows all non timeout message
queue exceptions. Should these not be passed to other catch handler?
- failuresPerMessage is incremented each failure... but this is a
process local variable... I thought we could this thing with multiple
processes?
- if no error queue we just drop the message in case of exception?
Should we not at least log something?
- what if error queue throws exception on send? failures per message
value is removed from queue prior to send. Object state gets crapped.
- posion messages if logger errors or error queue fails @line 367

@QueueIsLocal
- What if path is to domain name of machine? What if 127.0.0.1
instead of localhost. Maybe this should resolve name and check IP
address?

@GetTransactionTypeForSend/Receive
- Careful, Transaction.Current is thread static... but it's not clear
that GetTransactionTypeForSend should be treated as such.

@SetLocalQueue
- this.queue is set before queue is fully initialized. Should not set
member field till it's ready to be used.

@GetExtraTypes
- Looks like it doesn't get extra types, it removes the extra ones.
Strange way to name it.

@GetFullPath
- Params not validated... recommend adding exception if value is null
or split results in zero length array (avoids possible
NullReferenceException / IndexOutOfRangeException)

@OnFinishedMessageProcessing/OnStartedMessageProcessing
- lock event handlers? What if remove happens after check for null

@Dispose
- Seems like Transport should really have a Stop method, instead of
relying on Dispose to stop.
- Should queue be checked for null before Disposing
- Dispose does not follow best practices (bool disposing).
- Threads might continue to run forever since Stop doesn't actually
stop them... consider WaitHandler to signal / wait for actual stop?

Overall
- Class is not thread safe. Many properties and methods assume single
threaded model
- Grumble... Why do people ignore the standard .NET naming
conventions in public projects?


WorkerThread.cs

@Loop
- Looping requirement is bad IMO
- _toLock = pointless lock. (volatile bool doesn't need to be
protected by a lock)
- Should have lock around event handler call
- What if logger throws exception? Entire app will shut down due to
unhandled exception on thread.

@Start
- Not thread safe. Could result in multiple start.

@Stop
- Calling before stop makes thread stop automatically when it does
get started.
- You can never restart since StopRequested isn't ever cleared


Overall
- Why not just use actual threads (looks like point of class is just
to prevent rogue thread from shutting down service bus?). Real
threads would give ability to set priority, join, etc. all which you
can't do here since actual thread is hidden in private var.
- Ironic that even worker thread class, that replaces standard .NET
thread class functionality isn't thread safe and causes the same
problems it is intended to solve (ie. app crashes if logger fails).