WaveWriter can crash in dispose about cscore HOT 12 CLOSED

Using the _disposed syntax is considered best practice. It might be better to add the null-check right before the expression that causes this crash. Feels better, imho, and should cause less sideeffects we can't even think of at the moment.

from cscore.

Ok, then the fix requires a tiny bit more code, but doable.

Personally I do not agree with many best practices; after 25 years of fulltime software development, I see many "best practices" swing like a pendulum ;-)

from cscore.

So do I ;) But I was wondering why your pull request is in the queue for like two months now, and that was my guess. I encountered the same exception today, and came looking if some else already encountered this issue.

Looking at Dispose, I wonder if it would need more hardening. Alas, as far as I can see, there is no IsDisposing or IsDisposed in the WaveWriter. So when calling Dispose(), the caller needs to ensure that no thread calls Writer from the WaveWriter. And two threads should never access Dispose() at the same time, so the caller needs some more synching here.

But that are only whishes and just my two cents. All in all, it's an awesome library.

from cscore.

Oh, I even did not know that your classes were thread safe.

from cscore.

My classes? I'm not the author, just a random stranger. :)

from cscore.

Lol, sorry, I guess I was a bit too impulsive here ;-)

from cscore.

If the constructor crashes, there won't be any instance to dispose. Am I wrong?

from cscore.

Thanks for jumping in filoe.

Unfortunately you are - the finalizer will be called when you've got an exception in the constructor. Because there is no distinction between managed and unmanaged resources in Dispose() (that's the idea of the disposing var, as the comment says ;) ), an exception will be thrown.

As a bonus: For the user, it would be good to know if the object is in an "disposing" state, because then the user can block concurrent access to the write method - and it might take a while to write the header to the wav file on large files.* So a simple IsDisposing property would be awesome. And, for the sake of completeness, what will happen if I access the object after it being disposed? An IsDisposedwould make sense, at least from my point of view.

* Edit: Just realized you are already reserving the space for the header. Shouldn't depend on the filesize, just on plain i/o speed.

from cscore.

True, but the finalizer is called, so Dispose is called indirectly. I made a small test program to confirm this.

.NET always allocates the object (zeroed) before calling the constructor... So it will be finalized, since memory was allocated for it.

If you want to avoid these problems, then make a static method WaveWriter.FromFilePath instead. And just keep dummy constructors that cannot fail, or wrap the constructor bodies in try/catch, calling Dispose in the catch and rethrowing. Ugly, and could be avoided if the CLR would evaluate all arguments passed to a constructor before allocating an object, but helas...

from cscore.

Sometimes I even ended up with an enumeration and property LifeCycle { Allocated, Constructing, Constructed, Disposing, Disposed } ;-)

from cscore.

FYI, I've made a suggestion for a change to this class. Let me know your constructive feedback. 😃

from cscore.

Thanks for your contribution. Got merged :).

from cscore.