# Deleting an AudioFormatReader causes a crash **URL:** \ **Category:** Audio Plugins\ **Created:** [February 4, 2018, 10:16pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261 "2018-02-04T22:16:43Z")\ **Posts on this page:** 20\ **Page:** 1
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 4, 2018, 10:16pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/1 "2018-02-04T22:16:43Z")
I have a loop where quite a lot of AudioFormatReader objects are created using `AudioFormatManager.createReaderFor(InputStream)`. The docs for that method says: > If it returns a reader, it’s the caller’s responsibility to delete the reader. But if I delete the reader, either explicitly using `delete` or by wrapping it in a ScopedPointer, the program will crash. If I omit that part, everything appears to work fine. Although I’m a bit concerned that there’s a memory leak, since the docs explicitly tell me to delete it? ---
**Author:** ![daniel](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/daniel/32/790_2.png) [@daniel](https://forum.juce.com/u/daniel)\ **Post date:** [February 4, 2018, 10:19pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/2 "2018-02-04T22:19:43Z")
Depends what you are doing with the reader. If you are using it e.g. in an AudioFormatReaderSource, that will delete it on destruction (if you set the bool flag deleteReaderWhenThisIsDeleted to true). Maybe that’s the reason? ---
**Author:** ![xenakios](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/xenakios/32/4047_2.png) [@xenakios](https://forum.juce.com/u/xenakios)\ **Post date:** [February 4, 2018, 10:22pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/3 "2018-02-04T22:22:06Z")
Also the original poster might be putting them into an OwnedArray or such, which would take care of the deletion too. It would be great if people actually just posted the code they have written instead of explaining just a small portion of it… ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 4, 2018, 10:35pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/4 "2018-02-04T22:35:03Z")
Point taken, sorry about that! My code goes through a ZipFile of wav files and stores each of them as SamplerSound objects in an unordered\_map, with the name as key. Here’s my loop that goes through the ZipFile entries: ``` for (int i = 0; i < numEntries; i++) { const ZipFile::ZipEntry *entry = zipFile.getEntry(i); String name = entry->filename; if (name.endsWith(".wav") && samples[name] == nullptr) { InputStream *input = zipFile.createStreamForEntry(i); AudioFormatReader *reader = formatManager.createReaderFor(input); SamplerSound *sampSound = new SamplerSound("", *reader, noteRange, 0, 0.0, 0.0, 30.f); samples[name] = sampSound; } } ``` According to the docs for SamplerSound, it says the following about the AudioFormatReader argument to the constructor: > This object can be safely deleted by the caller after this constructor returns So this in combination with the docs for AudioFormatManager made me believe that I have to dispose of the AudioFormatReader myself. But if I do that in any of the ways mentioned above, the plugin host will crash. ---
**Author:** ![daniel](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/daniel/32/790_2.png) [@daniel](https://forum.juce.com/u/daniel)\ **Post date:** [February 4, 2018, 11:00pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/5 "2018-02-04T23:00:52Z")
That is indeed unexpected. A look at the sources shows, that the reader is just used in the constructor to read into an AudioSampleBuffer and is after that discarded, so there is no double ownership. What could have happened, since you probably deleted the InputStream as well, that you didn’t reverse the order, accidently deleting the InputStream before the reader? ```auto if (name.endsWith(".wav") && samples[name] == nullptr) { InputStream* input = zipFile.createStreamForEntry(i); ScopedPointer reader = formatManager.createReaderFor(input); SamplerSound *sampSound = new SamplerSound("", *reader, noteRange, 0, 0.0, 0.0, 30.f); reader.reset(); samples[name] = sampSound; } ``` N.B. `delete` and raw pointers should be fine too, but I like never to type delete 😉 Does that help? ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 4, 2018, 11:43pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/6 "2018-02-04T23:43:11Z")
Yeah, I try to avoid using `delete` as well, I just tried it now to see if it would make a difference. 🙂 My code seems to be working now! I must have messed something up, maybe I deleted the InputStream by mistake (I seem to remember wrapping it in a ScopedPointer at some point)? That would certainly cause a crash when deleting the AudioFormatReader, since it deletes the InputStream in its destructor… But everything is working fine now, so thanks! 🙂 ---
**Author:** ![daniel](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/daniel/32/790_2.png) [@daniel](https://forum.juce.com/u/daniel)\ **Post date:** [February 4, 2018, 11:47pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/7 "2018-02-04T23:47:49Z")
> [@nbacklund](#): > > That would certainly cause a crash when deleting the AudioFormatReader, since it deletes the InputStream in its destructor… Oh, thanks for not exposing me, sure that would have screwed my example code too 😃 I’ll correct that… ---
**Author:** ![godwincharan](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/godwincharan/32/213_2.png) [@godwincharan](https://forum.juce.com/u/godwincharan)\ **Post date:** [February 5, 2018, 6:04am UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/8 "2018-02-05T06:04:17Z")
> [@nbacklund](#): > > My code seems to be working now! I must have messed something up, maybe I deleted the InputStream by mistake (I seem to remember wrapping it in a ScopedPointer at some point)? That would certainly cause a crash when deleting the AudioFormatReader, since it deletes the InputStream in its destructor… > > But everything is working fine now, so thanks! 🙂 Also you might need to check for null. There might be occasion when you get reader as null. This might also cause crash. ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 5, 2018, 1:17pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/9 "2018-02-05T13:17:35Z")
> Also you might need to check for null. There might be occasion when you get reader as null. > This might also cause crash. Good call, thanks! Yes, my actual code has more checks in it. I stripped those and some other additional code (for instance progress reporting for the benefit of the UI) to keep the example clean. 🙂 In fact, I broke out most of the body of the loop to another function (`SampleSound *loadSample(ZipFile &zipFile, int entryIndex)`), because I actually have two loops: The first loop just loads the sounds that are needed urgently (based on settings), and the second loads the rest. This mechanism is there to make the plugin start producing sounds as quickly as possible. ---
**Author:** ![xenakios](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/xenakios/32/4047_2.png) [@xenakios](https://forum.juce.com/u/xenakios)\ **Post date:** [February 5, 2018, 1:23pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/10 "2018-02-05T13:23:42Z")
> [@nbacklund](#): > > This mechanism is there to make the plugin start producing sounds as quickly as possible. Hopefully you are doing all that in a thread safe manner. ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 5, 2018, 1:37pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/11 "2018-02-05T13:37:42Z")
> Hopefully you are doing all that in a thread safe manner. Yes, at least I think so. There is only ever one thread writing to the map (std::unordered\_map), the audio processing thread only reads from it. And I’ve made it so that nothing breaks if the sample lookup returns null, it just skips playback of that sample and tries again next time that sample is requested. Do you think I need something more serious than that, i e do you think there are cases where the map would return a pointer to a SamplerSound object that’s somehow invalid? I’m working under the assumption that the pointer is inserted into the map only when the SamplerSound constructor is complete. ---
**Author:** ![xenakios](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/xenakios/32/4047_2.png) [@xenakios](https://forum.juce.com/u/xenakios)\ **Post date:** [February 5, 2018, 1:42pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/12 "2018-02-05T13:42:59Z")
> [@nbacklund](#): > > > Hopefully you are doing all that in a thread safe manner. > > There is only ever one thread writing to the map (std::unordered\_map), the audio processing thread only reads from it. That’s not thread safe if you are not doing something to make it thread safe. (Like using a mutex.) ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 5, 2018, 2:18pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/13 "2018-02-05T14:18:50Z")
> That’s not thread safe if you are not doing something to make it thread safe. (Like using a mutex.) So that means that there are other possible outcomes than receiving a valid SamplerSound pointer or nullptr? Do you mean that the reading operation can mess with the writing operation in the other thread somehow? Would this be a correct way to go about it: I add `CriticalSection _mapAccess` as a member variable in my class (which is the only class that has access to the map since it’s a private member). In the method that does the writing: ``` for (int i = 0; i < numEntries; i++) { const ZipFile::ZipEntry *entry = zipFile.getEntry(i); String name = entry->filename; const ScopedLock lock(_mapAccess); if (_buffer[name] == nullptr && name.endsWith(".wav")) { _buffer[name] = loadSample(zipFile, i); } } ``` And in the method that does the reading: ``` SamplerSound *samplerSound = nullptr; { const ScopedLock lock(_mapAccess); samplerSound = _buffer[name]; } ``` I’m guessing the above code would do it? ---
**Author:** ![xenakios](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/xenakios/32/4047_2.png) [@xenakios](https://forum.juce.com/u/xenakios)\ **Post date:** [February 5, 2018, 2:25pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/14 "2018-02-05T14:25:45Z")
> [@nbacklund](#): > > > That’s not thread safe if you are not doing something to make it thread safe. (Like using a mutex.) > > So that means that there are other possible outcomes than receiving a valid SamplerSound pointer or nullptr? Do you mean that the reading operation can mess with the writing operation in the other thread somehow? It’s undefined behavior, so anything can happen, including the code working. The C++ standard library containers are not thread safe, so they must not be manipulated and/or read from multiple threads at the same time without synchronization. What might happen in practice in your use case is that the std::unordered\_map does a reallocation when you are assigning an element into it and the audio thread might then see the container in an inconsistent state. You might get back a null pointer, a valid pointer or something else in that case when reading from the container. But even without the reallocations happening it can lead to wrong results. ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 5, 2018, 2:28pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/15 "2018-02-05T14:28:36Z")
Got it, thank you for the explanation! ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 5, 2018, 10:54pm UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/16 "2018-02-05T22:54:35Z")
By the way, for the benefit of others who might come across this thread, I made a pretty serious mistake in my code above: I was loading the sample while holding the lock, which will of course slow down other threads considerably and completely unnecessarily… So it’s better to do something like: ``` for (int i = 0; i < numEntries; i++) { const ZipFile::ZipEntry *entry = zipFile.getEntry(i); const String &name = entry->filename; if (name.endsWith(".wav")) { SamplerSound *sampSound = loadSample(zipFile, i); if (sampSound != nullptr) { const ScopedLock lock(_mapAccess); _sample[name] = sampSound; } } } ``` ---
**Author:** ![daniel](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/daniel/32/790_2.png) [@daniel](https://forum.juce.com/u/daniel)\ **Post date:** [February 6, 2018, 12:02am UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/17 "2018-02-06T00:02:09Z")
Another addition, your unordered\_map doesn’t handle ownership and lifecycle. I don’t know how you handle the destruction, but whenever you remove or replace the pointer in the map, you must delete the pointee, otherwise you leak the SamplerSound objects. And when you do, you risk the SamplerSound still being used. For that purpose, the SamplerSound is a ReferenceCountedObject, and you can define the map like that, if you didn’t already: ``` std::unordered_map samples; ``` Now when you remove or replace a Ptr in the map, if it is the only reference, it will be destroyed. If it is used in playback, that code still prevents the deletion. Only if the reference there goes out of scope, it will be deleted. Only drawback: the deletion can happen on the audio thread. ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 6, 2018, 6:29am UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/18 "2018-02-06T06:29:31Z")
Right now I’m declaring it like this: ``` std::unordered_map> _samples; ``` I will never replace a SamplerSound in the map during the lifetime of the plugin. I just load them on startup, and they won’t be deleted until the map itself is deleted (i e when the plugin itself is destroyed). Is this safe enough, or should I still use SamplerSound::Ptr? ---
**Author:** ![jules](https://avatars.discourse-cdn.com/v4/letter/j/41988e/32.png) [@jules](https://forum.juce.com/u/jules)\ **Post date:** [February 6, 2018, 8:55am UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/19 "2018-02-06T08:55:04Z")
``` std::unordered_map> _samples; ``` Mixing ScopedPointers and ReferenceCountedPointers will definitely cause all kinds of dangling pointer bugs! You can’t safely use Strings in your audio callback, as they may allocate when copied. You _may_ be able to avoid race conditions if you’re very careful to use a read-only unordered\_map in an audio callback and are absolutely sure it won’t be modified at the same time. But std containers give no guarantees about what happens inside their methods, and you can’t assume that calling the operator won’t allocate (e.g. perhaps some versions of unordered\_map may occasionally decide to optimise their internal data structures when you call this method) (Oh, and leading underscores for names are generally considered bad style in C++ - they’re used by the standard library and for other reserved symbols.) So basically, nope! Don’t do it like that! ---
**Author:** ![nbacklund](https://sea2.discourse-cdn.com/flex026/user_avatar/forum.juce.com/nbacklund/32/2969_2.png) [@nbacklund](https://forum.juce.com/u/nbacklund)\ **Post date:** [February 6, 2018, 11:35am UTC](https://forum.juce.com/t/deleting-an-audioformatreader-causes-a-crash/26261/20 "2018-02-06T11:35:21Z")
Thank you for the feedback! I was considering whether to use ReferenceCountedPointer instead of ScopedPointer, not mixing them. 🙂 So how about this then: - Replace the string key with int - Use ReferenceCountedPointer instead of ScopedPointer The member variable declaration could look like this: ``` std::unordered_map samples; CriticalSection mapAccess; ``` And then I could lock concurrent access to the map like this: ``` // Writing in the initalization thread SamplerSound *sampSound = loadSample(...); if (sampSound != nullptr) { ScopedLock lock(mapAccess); samples[key] = sampSound; } ... // Reading in the audio processing thread SamplerSound *sampSound = nullptr; { ScopedLock lock(mapAccess); sampSound = samples[key]; } ``` Is this a better solution? If I have a ZipFile loaded into memory (via a MemoryInputStream), does it matter performance-wise in which order I stream the ZipEntries?