Repository: jguhlin/minimap2-rs (language: python)

Issue:
Issues with cloning of aligner
Hi @jguhlin,

  First; thanks again for the great work on this. I'm already finding minimap2-rs *super* useful, and we're looking to use it to include a "raw read" mode in our `oarfish` tool.  I noticed two specific issues with cloning an aligner and wondered if you had any thoughts or insight into them.

  So I'm following the basic strategy employed in fake minimap2 / [minimap2-rs-test](https://t.co/93KFQpwLjW), where I have several threads that take reads (in my case over a channel) and then send the reads to another consumer.  Each thread needs it's own aligner object and I am calling `clone()` to get it.

  I'm having two problems.  First, for context, since I need references to local variables, I am using a `std::thread::scope` and scoped threads which may be why I see this clearly in my example but the other examples I'm building off of don't.

  1) Since each clone gets it's own pointer to the index (the `idx` field), they all try to `drop` the index (which calls `free` in the FFI) when the thread is joined and the scope ends.  However, this obviously leads to a double-free and, in my case, a segfault.  I can get around this by explicitly setting the `idx` field of each cloned `Aligner` to `None` before the thread returns, but this feels error prone.  I wonder if it would make sense to have either (a) some properly resource-managed pointer type for the index (e.g. an `Arc` or some such), or (b) some specific idx "view" type that essentially constitutes a weakptr to the original index.  Either way, it'd be great to have some sort of way to do the clone in a carefree manner but not worry about double-free.

  2) It's not clear to me the "depth" of the clone.  Specifically, while in the original aligner I call the `with_cigar` method on the builder, the clones aren't seeming to provide this information by default and instead I'm having to call `aligner.clone().with_cigar()` to get a clone that also provides CIGAR strings.  I guess the expected cloning behavior is for the clone to have all of the same properties set as the original, and the real fear I have here is that I don't know what other fields might be affected apart from the CIGAR string generation (perhaps it's simply the flags that aren't deep cloned)?


Anyway, I'm happy to provide further detail about the above, or to discuss potential solutions or design decisions about addressing these.  Let me know your thoughts, and thanks again for minimap2-rs!

--Rob
Issues with cloning of aligner
Hi @jguhlin,

  First; thanks again for the great work on this. I'm already finding minimap2-rs *super* useful, and we're looking to use it to include a "raw read" mode in our `oarfish` tool.  I noticed two specific issues with cloning an aligner and wondered if you had any thoughts or insight into them.

  So I'm following the basic strategy employed in fake minimap2 / [minimap2-rs-test](https://t.co/93KFQpwLjW), where I have several threads that take reads (in my case over a channel) and then send the reads to another consumer.  Each thread needs it's own aligner object and I am calling `clone()` to get it.

  I'm having two problems.  First, for context, since I need references to local variables, I am using a `std::thread::scope` and scoped threads which may be why I see this clearly in my example but the other examples I'm building off of don't.

  1) Since each clone gets it's own pointer to the index (the `idx` field), they all try to `drop` the index (which calls `free` in the FFI) when the thread is joined and the scope ends.  However, this obviously leads to a double-free and, in my case, a segfault.  I can get around this by explicitly setting the `idx` field of each cloned `Aligner` to `None` before the thread returns, but this feels error prone.  I wonder if it would make sense to have either (a) some properly resource-managed pointer type for the index (e.g. an `Arc` or some such), or (b) some specific idx "view" type that essentially constitutes a weakptr to the original index.  Either way, it'd be great to have some sort of way to do the clone in a carefree manner but not worry about double-free.

  2) It's not clear to me the "depth" of the clone.  Specifically, while in the original aligner I call the `with_cigar` method on the builder, the clones aren't seeming to provide this information by default and instead I'm having to call `aligner.clone().with_cigar()` to get a clone that also provides CIGAR strings.  I guess the expected cloning behavior is for the clone to have all of the same properties set as the original, and the real fear I have here is that I don't know what other fields might be affected apart from the CIGAR string generation (perhaps it's simply the flags that aren't deep cloned)?


Anyway, I'm happy to provide further detail about the above, or to discuss potential solutions or design decisions about addressing these.  Let me know your thoughts, and thanks again for minimap2-rs!

--Rob
Issues with cloning of aligner
Hi @jguhlin,

  First; thanks again for the great work on this. I'm already finding minimap2-rs *super* useful, and we're looking to use it to include a "raw read" mode in our `oarfish` tool.  I noticed two specific issues with cloning an aligner and wondered if you had any thoughts or insight into them.

  So I'm following the basic strategy employed in fake minimap2 / [minimap2-rs-test](https://t.co/93KFQpwLjW), where I have several threads that take reads (in my case over a channel) and then send the reads to another consumer.  Each thread needs it's own aligner object and I am calling `clone()` to get it.

  I'm having two problems.  First, for context, since I need references to local variables, I am using a `std::thread::scope` and scoped threads which may be why I see this clearly in my example but the other examples I'm building off of don't.

  1) Since each clone gets it's own pointer to the index (the `idx` field), they all try to `drop` the index (which calls `free` in the FFI) when the thread is joined and the scope ends.  However, this obviously leads to a double-free and, in my case, a segfault.  I can get around this by explicitly setting the `idx` field of each cloned `Aligner` to `None` before the thread returns, but this feels error prone.  I wonder if it would make sense to have either (a) some properly resource-managed pointer type for the index (e.g. an `Arc` or some such), or (b) some specific idx "view" type that essentially constitutes a weakptr to the original index.  Either way, it'd be great to have some sort of way to do the clone in a carefree manner but not worry about double-free.

  2) It's not clear to me the "depth" of the clone.  Specifically, while in the original aligner I call the `with_cigar` method on the builder, the clones aren't seeming to provide this information by default and instead I'm having to call `aligner.clone().with_cigar()` to get a clone that also provides CIGAR strings.  I guess the expected cloning behavior is for the clone to have all of the same properties set as the original, and the real fear I have here is that I don't know what other fields might be affected apart from the CIGAR string generation (perhaps it's simply the flags that aren't deep cloned)?


Anyway, I'm happy to provide further detail about the above, or to discuss potential solutions or design decisions about addressing these.  Let me know your thoughts, and thanks again for minimap2-rs!

--Rob


Discussion / hints:
It's my first FFI project, so has been a learning experience. I like Arc as a solution.

Surprised about the second point, I'll dig into it this week! Clone should produce full clones.
Hi @rob-p hope you don't mind me jumping in,

I am having the same issue you outline in your first point (I am using crossbeam channels in a similar way to the "fake minimap2") - but after cloning the `Aligner` and joining the threads I get a double free segfault.

I tried doing something like `aligner.idx = None;` in the thread after calling `aligner.map` but I still get a segfault - do you have an example of how you implemented the workaround/multithreading?

Cheers

Hi @Sam-Sims,

  You can find the relevant line (and closure) in my code here https://github.com/COMBINE-lab/oarfish/blob/5b09b4345b609559a1aa7d76df75bf3aebac6ba9/src/bulk.rs#L334.  Basically, you only let the original aligner retain the pointer and make sure you set it to None before the point of dropping within each thread.  Let me know if this helps.

--Rob
Ah thanks that's super useful! I needed to set it to None in the thread closure. Agree it would be nice to be able to clone an`Aligner` without having to worry about a segfault.
I have something which *seems* to be working well (i.e., I haven't got any seqfaults etc.)

Essentially I created an AlignerWrapper which holds an `Arc` to an `Aligner`, then I [`Arc::clone`](https://doc.rust-lang.org/std/sync/struct.Arc.html#method.clone) the aligner in each spawned thread. Here is some example code

```rust
struct AlignerWrapper {
    /// Shared aligner across threads
    aligner: Arc<Aligner>, 
}

impl AlignerWrapper {
    fn new(target_file: &Path, threads: usize) -> Result<Self, LrgeError> {
        let mut aligner = Aligner::builder()
            .ava_ont()
            .with_index_threads(threads)
            .with_index(target_file, None)
            .unwrap();

        Ok(Self {
            aligner: Arc::new(aligner),
        })
    }

    fn align_reads(&self, query_file: PathBuf) -> Result<Vec<Mapping>, LrgeError> {
        // Bounded channel to control memory usage
        let (sender, receiver) = channel::bounded(1000); 

        // Shared reference for the producer thread
        let aligner = Arc::clone(&self.aligner); 

        // Producer: Read FASTQ records and send them to the channel
        let producer = std::thread::spawn(move || -> Result<(), LrgeError> {
            let mut fastx_reader = parse_fastx_file(query_file).map_err(|e| {
                LrgeError::FastqParseError(format!("Error parsing query FASTQ file: {}", e))
            })?;

            while let Some(record) = fastx_reader.next() {
                match record {
                    Ok(rec) => {
                        let msg = io::Message::Data((
                            rec.read_id().to_owned(),
                            Vec::from(rec.seq().to_owned()),
                        ));
                        if sender.send(msg).is_err() {
                            break; // Exit if the receiver is dropped
                        }
                    }
                    Err(e) => {
                        return Err(LrgeError::FastqParseError(format!(
                            "Error parsing query FASTQ file: {}",
                            e
                        )));
                    }
                }
            }

            // Close the channel to signal that no more records will be sent
            drop(sender);

            Ok(())
        });

        // Consumer: Process records from the channel in parallel
        let alignments: Vec<Mapping> = receiver
            .into_iter()
            .par_bridge()  // gosh rayon is ridiculously ergonomic
            .flat_map(|record| {
                let (rid, seq) = match record {
                    io::Message::Data(data) => data,
                    io::Message::End => unimplemented!("End message not expected here"),
                };
                debug!("Processing read: {:?}", String::from_utf8_lossy(&rid));
                let mut qname = rid.to_owned();
                if !(qname.last() == Some(&0)) {
                    qname.push(0);
                }
                // Use the shared aligner to perform alignment
                aligner.map(&seq, Some(&rid)).unwrap()
            })
            .collect();

        // Wait for the producer to finish
        producer.join().unwrap();

        Ok(alignments)
    }
}
```

then you can use it something like

```rust
let aligner = AlignerWrapper::new(&target_file, self.threads)?;
let alignments = aligner.align_reads(query_file)?;

for mapping in alignments {
    println!("{:?}", mapping);
}
```

Note, this is using a small API wrapper I have written around minimap-sys so some calls to things like `map` are a little different. But hopefully you get the gist.

I'd be interested to know if this works for others or if I am just not seeing the problem pop up yet and need to do more thorough stress testing.
Hi @mbhall88,

  I believe your specific architecture should work, since you have only one underlying `Aligner` object that is accessed through the `Arc`. Rather @Sam-Sims and I were attempting to use the approach shown in the [example application](https://github.com/jianshu93/minimap2-rs-test/blob/main/src/main.rs), where each consumer (i.e. mapping) thread makes it's own clone of the `Aligner` struct.  Granted, I don't know what effect the extra `Arc` indirection will have on performance, but the idea of having a local clone in each thread is so that the `Aligner`'s avoid the extra redirection, and also (perhaps) so they can have some different (non-index-related) settings if one wants.

  If your architecture works well in your use case, I believe it seems sound to me.  However, I still think it would be useful if we could avoid the undefined behavior (i.e. use-after free) that occurs when a cloned `Aligner` is used in different scopes.  There, the end of the first closing scope drops the `Aligner` and destroys the underlying index pointer.

--Rob
I did take a stab at this a while ago, implementing a new struct for the index - `MmIdx`, and then using an `Arc` to manage it

Something like:
```rust
pub struct MmIdx(*mut mm_idx_t);
impl Drop for MmIdx {
    fn drop(&mut self) {
        unsafe { mm_idx_destroy(self.0) };
    }
}
```
```rust
pub struct Aligner {
    ...
    pub idx: Option<Arc<MmIdx>>,
}
```

and then removing the original drop implementation for the aligner. 

I just pushed the changes here - I wasnt 100% sure on the approach: https://github.com/Sam-Sims/minimap2-rs/commit/ad01bbfcb229dba8e6db28081e7f22d1936910f6

This did work for me on my own use case but I didn't have time to properly test it. If that approach seems reasonable I would be happy to open a PR


hi @rob-p ,

Thanks for debugging my fake minimap2-rs-test, when I started to use it in some large projects, this double free error kept showing up: main(25070,0x16bc4f000) malloc: double free for ptr 0x1783a8000

I am wondering in my case, what would be the best way to get rid of it. I tried to add aligner.idx = None; within each consumer thread without success. 

Thanks,
Best,
Jianshu
never mind, I've solved it. I also udpated the fake minimap2-rs-test. 

Thanks,

Jianshu
It's my first FFI project, so has been a learning experience. I like Arc as a solution.

Surprised about the second point, I'll dig into it this week! Clone should produce full clones.
Hi @rob-p hope you don't mind me jumping in,

I am having the same issue you outline in your first point (I am using crossbeam channels in a similar way to the "fake minimap2") - but after cloning the `Aligner` and joining the threads I get a double free segfault.

I tried doing something like `aligner.idx = None;` in the thread after calling `aligner.map` but I still get a segfault - do you have an example of how you implemented the workaround/multithreading?

Cheers

Hi @Sam-Sims,

  You can find the relevant line (and closure) in my code here https://github.com/COMBINE-lab/oarfish/blob/5b09b4345b609559a1aa7d76df75bf3aebac6ba9/src/bulk.rs#L334.  Basically, you only let the original aligner retain the pointer and make sure you set it to None before the point of dropping within each thread.  Let me know if this helps.

--Rob
Ah thanks that's super useful! I needed to set it to None in the thread closure. Agree it would be nice to be able to clone an`Aligner` without having to worry about a segfault.
I have something which *seems* to be working well (i.e., I haven't got any seqfaults etc.)

Essentially I created an AlignerWrapper which holds an `Arc` to an `Aligner`, then I [`Arc::clone`](https://doc.rust-lang.org/std/sync/struct.Arc.html#method.clone) the aligner in each spawned thread. Here is some example code

```rust
struct AlignerWrapper {
    /// Shared aligner across threads
    aligner: Arc<Aligner>, 
}

impl AlignerWrapper {
    fn new(target_file: &Path, threads: usize) -> Result<Self, LrgeError> {
        let mut aligner = Aligner::builder()
            .ava_ont()
            .with_index_threads(threads)
            .with_index(target_file, None)
            .unwrap();

        Ok(Self {
            aligner: Arc::new(aligner),
        })
    }

    fn align_reads(&self, query_file: PathBuf) -> Result<Vec<Mapping>, LrgeError> {
        // Bounded channel to control memory usage
        let (sender, receiver) = channel::bounded(1000); 

        // Shared reference for the producer thread
        let aligner = Arc::clone(&self.aligner); 

        // Producer: Read FASTQ records and send them to the channel
        let producer = std::thread::spawn(move || -> Result<(), LrgeError> {
            let mut fastx_reader = parse_fastx_file(query_file).map_err(|e| {
                LrgeError::FastqParseError(format!("Error parsing query FASTQ file: {}", e))
            })?;

            while let Some(record) = fastx_reader.next() {
                match record {
                    Ok(rec) => {
                        let msg = io::Message::Data((
                            rec.read_id().to_owned(),
                            Vec::from(rec.seq().to_owned()),
                        ));
                        if sender.send(msg).is_err() {
                            break; // Exit if the receiver is dropped
                        }
                    }
                    Err(e) => {
                        return Err(LrgeError::FastqParseError(format!(
                            "Error parsing query FASTQ file: {}",
                            e
                        )));
                    }
                }
            }

            // Close the channel to signal that no more records will be sent
            drop(sender);

            Ok(())
        });

        // Consumer: Process records from the channel in parallel
        let alignments: Vec<Mapping> = receiver
            .into_iter()
            .par_bridge()  // gosh rayon is ridiculously ergonomic
            .flat_map(|record| {
                let (rid, seq) = match record {
                    io::Message::Data(data) => data,
                    io::Message::End => unimplemented!("End message not expected here"),
                };
                debug!("Processing read: {:?}", String::from_utf8_lossy(&rid));
                let mut qname = rid.to_owned();
                if !(qname.last() == Some(&0)) {
                    qname.push(0);
                }
                // Use the shared aligner to perform alignment
                aligner.map(&seq, Some(&rid)).unwrap()
            })
            .collect();

        // Wait for the producer to finish
        producer.join().unwrap();

        Ok(alignments)
    }
}
```

then you can use it something like

```rust
let aligner = AlignerWrapper::new(&target_file, self.threads)?;
let alignments = aligner.align_reads(query_file)?;

for mapping in alignments {
    println!("{:?}", mapping);
}
```

Note, this is using a small API wrapper I have written around minimap-sys so some calls to things like `map` are a little different. But hopefully you get the gist.

I'd be interested to know if this works for others or if I am just not seeing the problem pop up yet and need to do more thorough stress testing.
Hi @mbhall88,

  I believe your specific architecture should work, since you have only one underlying `Aligner` object that is accessed through the `Arc`. Rather @Sam-Sims and I were attempting to use the approach shown in the [example application](https://github.com/jianshu93/minimap2-rs-test/blob/main/src/main.rs), where each consumer (i.e. mapping) thread makes it's own clone of the `Aligner` struct.  Granted, I don't know what effect the extra `Arc` indirection will have on performance, but the idea of having a local clone in each thread is so that the `Aligner`'s avoid the extra redirection, and also (perhaps) so they can have some different (non-index-related) settings if one wants.

  If your architecture works well in your use case, I believe it seems sound to me.  However, I still think it would be useful if we could avoid the undefined behavior (i.e. use-after free) that occurs when a cloned `Aligner` is used in different scopes.  There, the end of the first closing scope drops the `Aligner` and destroys the underlying index pointer.

--Rob
I did take a stab at this a while ago, implementing a new struct for the index - `MmIdx`, and then using an `Arc` to manage it

Something like:
```rust
pub struct MmIdx(*mut mm_idx_t);
impl Drop for MmIdx {
    fn drop(&mut self) {
        unsafe { mm_idx_destroy(self.0) };
    }
}
```
```rust
pub struct Aligner {
    ...
    pub idx: Option<Arc<MmIdx>>,
}
```

and then removing the original drop implementation for the aligner. 

I just pushed the changes here - I wasnt 100% sure on the approach: https://github.com/Sam-Sims/minimap2-rs/commit/ad01bbfcb229dba8e6db28081e7f22d1936910f6

This did work for me on my own use case but I didn't have time to properly test it. If that approach seems reasonable I would be happy to open a PR


hi @rob-p ,

Thanks for debugging my fake minimap2-rs-test, when I started to use it in some large projects, this double free error kept showing up: main(25070,0x16bc4f000) malloc: double free for ptr 0x1783a8000

I am wondering in my case, what would be the best way to get rid of it. I tried to add aligner.idx = None; within each consumer thread without success. 

Thanks,
Best,
Jianshu
never mind, I've solved it. I also udpated the fake minimap2-rs-test. 

Thanks,

Jianshu
It's my first FFI project, so has been a learning experience. I like Arc as a solution.

Surprised about the second point, I'll dig into it this week! Clone should produce full clones.
Hi @rob-p hope you don't mind me jumping in,

I am having the same issue you outline in your first point (I am using crossbeam channels in a similar way to the "fake minimap2") - but after cloning the `Aligner` and joining the threads I get a double free segfault.

I tried doing something like `aligner.idx = None;` in the thread after calling `aligner.map` but I still get a segfault - do you have an example of how you implemented the workaround/multithreading?

Cheers

Hi @Sam-Sims,

  You can find the relevant line (and closure) in my code here https://github.com/COMBINE-lab/oarfish/blob/5b09b4345b609559a1aa7d76df75bf3aebac6ba9/src/bulk.rs#L334.  Basically, you only let the original aligner retain the pointer and make sure you set it to None before the point of dropping within each thread.  Let me know if this helps.

--Rob
Ah thanks that's super useful! I needed to set it to None in the thread closure. Agree it would be nice to be able to clone an`Aligner` without having to worry about a segfault.
I have something which *seems* to be working well (i.e., I haven't got any seqfaults etc.)

Essentially I created an AlignerWrapper which holds an `Arc` to an `Aligner`, then I [`Arc::clone`](https://doc.rust-lang.org/std/sync/struct.Arc.html#method.clone) the aligner in each spawned thread. Here is some example code

```rust
struct AlignerWrapper {
    /// Shared aligner across threads
    aligner: Arc<Aligner>, 
}

impl AlignerWrapper {
    fn new(target_file: &Path, threads: usize) -> Result<Self, LrgeError> {
        let mut aligner = Aligner::builder()
            .ava_ont()
            .with_index_threads(threads)
            .with_index(target_file, None)
            .unwrap();

        Ok(Self {
            aligner: Arc::new(aligner),
        })
    }

    fn align_reads(&self, query_file: PathBuf) -> Result<Vec<Mapping>, LrgeError> {
        // Bounded channel to control memory usage
        let (sender, receiver) = channel::bounded(1000); 

        // Shared reference for the producer thread
        let aligner = Arc::clone(&self.aligner); 

        // Producer: Read FASTQ records and send them to the channel
        let producer = std::thread::spawn(move || -> Result<(), LrgeError> {
            let mut fastx_reader = parse_fastx_file(query_file).map_err(|e| {
                LrgeError::FastqParseError(format!("Error parsing query FASTQ file: {}", e))
            })?;

            while let Some(record) = fastx_reader.next() {
                match record {
                    Ok(rec) => {
                        let msg = io::Message::Data((
                            rec.read_id().to_owned(),
                            Vec::from(rec.seq().to_owned()),
                        ));
                        if sender.send(msg).is_err() {
                            break; // Exit if the receiver is dropped
                        }
                    }
                    Err(e) => {
                        return Err(LrgeError::FastqParseError(format!(
                            "Error parsing query FASTQ file: {}",
                            e
                        )));
                    }
                }
            }

            // Close the channel to signal that no more records will be sent
            drop(sender);

            Ok(())
        });

        // Consumer: Process records from the channel in parallel
        let alignments: Vec<Mapping> = receiver
            .into_iter()
            .par_bridge()  // gosh rayon is ridiculously ergonomic
            .flat_map(|record| {
                let (rid, seq) = match record {
                    io::Message::Data(data) => data,
                    io::Message::End => unimplemented!("End message not expected here"),
                };
                debug!("Processing read: {:?}", String::from_utf8_lossy(&rid));
                let mut qname = rid.to_owned();
                if !(qname.last() == Some(&0)) {
                    qname.push(0);
                }
                // Use the shared aligner to perform alignment
                aligner.map(&seq, Some(&rid)).unwrap()
            })
            .collect();

        // Wait for the producer to finish
        producer.join().unwrap();

        Ok(alignments)
    }
}
```

then you can use it something like

```rust
let aligner = AlignerWrapper::new(&target_file, self.threads)?;
let alignments = aligner.align_reads(query_file)?;

for mapping in alignments {
    println!("{:?}", mapping);
}
```

Note, this is using a small API wrapper I have written around minimap-sys so some calls to things like `map` are a little different. But hopefully you get the gist.

I'd be interested to know if this works for others or if I am just not seeing the problem pop up yet and need to do more thorough stress testing.
Hi @mbhall88,

  I believe your specific architecture should work, since you have only one underlying `Aligner` object that is accessed through the `Arc`. Rather @Sam-Sims and I were attempting to use the approach shown in the [example application](https://github.com/jianshu93/minimap2-rs-test/blob/main/src/main.rs), where each consumer (i.e. mapping) thread makes it's own clone of the `Aligner` struct.  Granted, I don't know what effect the extra `Arc` indirection will have on performance, but the idea of having a local clone in each thread is so that the `Aligner`'s avoid the extra redirection, and also (perhaps) so they can have some different (non-index-related) settings if one wants.

  If your architecture works well in your use case, I believe it seems sound to me.  However, I still think it would be useful if we could avoid the undefined behavior (i.e. use-after free) that occurs when a cloned `Aligner` is used in different scopes.  There, the end of the first closing scope drops the `Aligner` and destroys the underlying index pointer.

--Rob
I did take a stab at this a while ago, implementing a new struct for the index - `MmIdx`, and then using an `Arc` to manage it

Something like:
```rust
pub struct MmIdx(*mut mm_idx_t);
impl Drop for MmIdx {
    fn drop(&mut self) {
        unsafe { mm_idx_destroy(self.0) };
    }
}
```
```rust
pub struct Aligner {
    ...
    pub idx: Option<Arc<MmIdx>>,
}
```

and then removing the original drop implementation for the aligner. 

I just pushed the changes here - I wasnt 100% sure on the approach: https://github.com/Sam-Sims/minimap2-rs/commit/ad01bbfcb229dba8e6db28081e7f22d1936910f6

This did work for me on my own use case but I didn't have time to properly test it. If that approach seems reasonable I would be happy to open a PR


hi @rob-p ,

Thanks for debugging my fake minimap2-rs-test, when I started to use it in some large projects, this double free error kept showing up: main(25070,0x16bc4f000) malloc: double free for ptr 0x1783a8000

I am wondering in my case, what would be the best way to get rid of it. I tried to add aligner.idx = None; within each consumer thread without success. 

Thanks,
Best,
Jianshu
never mind, I've solved it. I also udpated the fake minimap2-rs-test. 

Thanks,

Jianshu

Fix patch:
diff --git a/.gitignore b/.gitignore
index 4cde53c..2194cfd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,3 +7,4 @@ test_data/*.bam
 test_data/*.mmi
 .ipynb_checkpoints
 .vscode/
+.quarto
\ No newline at end of file
diff --git a/Cargo.toml b/Cargo.toml
index 1feaa74..2e84093 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -50,13 +50,14 @@ rayon = "1.10"
 
 [features]
 default = ["map-file"]
-sse2only = ["minimap2-sys/sse2only"]
+map-file = ["needletail"] #, "simdutf8"]
 htslib = ['rust-htslib']
 simde = ["minimap2-sys/simde"]
-map-file = ["needletail"] #, "simdutf8"]
 zlib-ng = ["minimap2-sys/zlib-ng"]
 curl = ["rust-htslib/curl"]
 static = ["minimap2-sys/static", "rust-htslib/static"]
+sse2only = ["minimap2-sys/sse2only"]
+
 
 [package.metadata.docs.rs]
 features = ["map-file", "htslib"]
diff --git a/README.md b/README.md
index 46f20db..9ab9112 100644
--- a/README.md
+++ b/README.md
@@ -6,20 +6,21 @@ A rust FFI library for [minimap2](https://github.com/lh3/minimap2/). In developm
 [![codecov](https://codecov.io/gh/jguhlin/minimap2-rs/branch/main/graph/badge.svg?token=huw27ZC6Qy)](https://codecov.io/gh/jguhlin/minimap2-rs)
 
 # Structure
-minimap2-sys is the library of the raw FFI bindings to minimap2. minimap2 is the more rusty version.
+minimap2-sys is the raw FFI bindings to minimap2. minimap2 is the more opinionated, rusty version.
 
 # How to use
 ## Requirements
 ```toml
-minimap2 = "0.1.20+minimap2.2.28"
+minimap2 = "0.1.21+minimap2.2.28"
 ```
 Also see [Features](#features)
 
-Tested with rustc 1.64.0 and nightly. So probably a good idea to upgrade before running. But let me know if you run into pain points with older versions and will try to fix!
+Tested with rustc 1.82.0 and nightly. So probably a good idea to upgrade before running. But let me know if you run into pain points with older versions and will try to fix.
 
 ## Minimap2 Version Table
 | minimap2-rs | minimap2 |
 |-------------|----------|
+| 0.1.21      | 2.28     |
 | 0.1.20      | 2.28     |
 | 0.1.19      | 2.28     |
 | 0.1.18      | 2.28     |
@@ -42,7 +43,7 @@ Align a sequence:
 ```rust
 let seq: Vec<u8> = b"ACTGACTCACATCGACTACGACTACTAGACACTAGACTATCGACTACTGACATCGA";
 let alignment = aligner
-    .map(&seq, false, false, None, None)
+    .map(&seq, false, false, None, None, Some(b"My Sequence Name"))
     .expect("Unable to align");
 ```
 
@@ -53,6 +54,9 @@ let aligner = map_ont();
 let aligner = asm20();
 ```
 
+**Note** Each preset overwrites different arguments. Using multiple at a time is not technically supported, but will work. Results unknown. So be careful!
+It's equivalent to running minimap2 -x map_ont -x short ...
+
 ### Customization
 [MapOpts](https://docs.rs/minimap2-sys/0.1.5/minimap2_sys/struct.mm_mapopt_t.html) and [IdxOpts](https://docs.rs/minimap2-sys/0.1.5/minimap2_sys/struct.mm_idxopt_t.html) can be customized with Rust's struct pattern, as well as applying mapping settings. Inspired by [bevy](https://bevyengine.org/).
 ```rust
@@ -83,7 +87,7 @@ let mut fasta = Fasta::from_buffer(&mut reader)
 for seq in reader {
     let seq = seq.unwrap();
     let alignment: Vec<Mapping> = aligner
-        .map(&seq.sequence.unwrap(), false, false, None, None)
+        .map(&seq.sequence.unwrap(), false, false, None, None, None)
         .expect("Unable to align");
     println!("{:?}", alignment);
 }
@@ -111,17 +115,19 @@ This _appears_ to work.
 use rayon::prelude::*;
 
 let results = sequences.par_iter().map(|seq| {
-    aligner.map(seq.as_bytes(), false, false, None, None).unwrap()
+    aligner.map(seq.as_bytes(), false, false, None, None, None).unwrap()
 }).collect::<Vec<_>>();
 ```
 
 ## Features
 The following crate features are available:
-* `mm2-fast` - Replace minimap2 with [mm2-fast](https://github.com/bwa-mem2/mm2-fast). This is likely not portable.
-* `htslib` - Support output of bam/sam files using htslib.
-* `simde` - Compile minimap2 / mm2-fast with [simd-everywhere](https://github.com/simd-everywhere/simde) support. 
-* `map-file` - *Default* - Convenience function for mapping an entire file. Caution, this is single-threaded. 
-* `sse2only` - Compiles for SSE2 support only (Default is to try to compile for SSE4.1, SSE2 only is default on aarch64)
+* map-file - Enables the ability to map a file directly to a reference. Enabled by deafult
+* htslib - Provides an interface to minimap2 that returns rust_htslib::Records
+* simde - Enables SIMD Everywhere library in minimap2
+* zlib-ng - Enables the use of zlib-ng for faster compression
+* curl - Enables curl for htslib
+* static - Builds minimap2 as a static library
+* sse2only - Builds minimap2 with only SSE2 support
 
 Map-file is a *default* feature and enabled unless otherwise specified.
 
@@ -129,7 +135,7 @@ Map-file is a *default* feature and enabled unless otherwise specified.
 * setting mismatch penalty for base transitions [minimap 2.27 release notes](https://github.com/lh3/minimap2/releases/tag/v2.27)
 * Generate ds tags to indicate uncertainty in indels
 
-Potentially more, but I'm using this to keep track. I'd expect those would get implemented over time, but if you have urgent need open a pull request or an issue! Thanks
+Potentially others. Please create an issue! 
 
 ## Building for MUSL
 Follow these [instructions](https://github.com/rust-cross/rust-musl-cross#prebuilt-images).
@@ -141,10 +147,9 @@ alias rust-musl-builder='docker run --rm -it -v "$(pwd)":/home/rust/src messense
 rust-musl-builder cargo build --release
 ```
 
-Please note minimap2 is only tested for x86_64. Other platforms may work, please open an issue if minimap2 compiles but minimap2-rs does not.
+Minimap2 is tested on x86_64 and aarch64 (arm64). Other platforms may work, please open an issue if minimap2 compiles but minimap2-rs does not.
 
 ### Features tested with MUSL
-* `mm2-fast` - **Fail**
 * `htslib` - **Success**
 * `simde` - **Success**
 
@@ -153,23 +158,19 @@ Please note minimap2 is only tested for x86_64. Other platforms may work, please
 - [mappy-rs](https://github.com/Adoni5/mappy-rs) - Drop-in multi-threaded replacement for python's mappy
 - [HiFiHLA](https://github.com/PacificBiosciences/hifihla) - HLA star-calling tool for PacBio HiFi data
 - [STRdust](https://github.com/wdecoster/STRdust) - Tandem repeat genotyper for long reads
-
-# Want feedback
-* Many fields are i32 / i8 to mimic the C environment, but would it make more sense to convert to u32 / u8 / usize?
-* Let me know pain points!
+- [oarfish](https://github.com/COMBINE-lab/oarfish) - transcript quantification from long-read RNA-seq data
 
 # Next things todo
-* Print other tags so we can have an entire PAF format
-* -sys Compile with SSE2 / SSE4.1 (auto-detect, but also make with features)
 * Multi-thread guide (tokio async threads or use crossbeam queue and traditional threads?)
 * Iterator interface for map_file
-* MORE TESTS
-* -sys Get SSE working with "sse" feature (compiles and tests work in -sys crate, but not main crate)
 * -sys Possible to decouple from pthread?
-* -sys Enable Lisa-hash for mm2-fast? But must handle build arguments from the command-line.
 
 # Citation
-You should cite the minimap2 papers if you use this in your work.
+Please cite the appropriate minimap2 papers if you use this in your work, as well as this library.
+
+## DOI for this library
+
+## Minimap2 Papers
 
 > Li, H. (2018). Minimap2: pairwise alignment for nucleotide sequences.
 > *Bioinformatics*, **34**:3094-3100. [doi:10.1093/bioinformatics/bty191][doi]
@@ -180,6 +181,29 @@ and/or:
 > *Bioinformatics*, **37**:4572-4574. [doi:10.1093/bioinformatics/btab705][doi2]
 
 # Changelog
+### 0.1.21 minimap2 2.28
+Contributors to this release: @mbhall88 @rob-p @Sam-Sims @charlesgregory @PB-DB
+#### Breaking Changes
++ Map now returns Arc String's to reduce memory allocation for large and/or repetitive jobs
++ map now takes an additional argument, query_name: Option<impl AsRef<[u8]>>, possibly solves [#75](https://github.com/jguhlin/minimap2-rs/issues/75) (@rob-p @mbhall88 @jguhlin)
++ Arc the Index, to prevent double-frees, solves [#71](https://github.com/jguhlin/minimap2-rs/issues/71)
++ Map file now passes in query name, which should help with [#75](https://github.com/jguhlin/minimap2-rs/issues/75)
++ Supplementary flag now better detected (@rob-p)
++ FIX: Cigar string missing softclip operation (@Sam-Sims)
+
+### Other Changes
++ Add ergonomic functions n_seq and get_seq.
++ Better docs on applying presets, solves [#84](https://github.com/jguhlin/minimap2-rs/issues/84)
++ Better detection of target arch c_char's and ptr's, solves [#82](https://github.com/jguhlin/minimap2-rs/issues/82)
++ Support for M1 Mac compilation and addition of github workflows to test it, solving [#81](https://github.com/jguhlin/minimap2-rs/issues/81)
++ Rayon test, so some support, closes [#5](https://github.com/jguhlin/minimap2-rs/issues/5)
++ Static str's and now static CStr's
++ FIX: memory leak due to sequences allocated by minimap2 not being freed @charlesgregory
++ Add Send + Sync to Aligner, along with unit test @PB-DB
++ Only use rust-htslib/curl when curl feature is enabled @PB-DB
++ Mark bam::Record objects as supplementary @PB-DB
++ Experimental Android support (tested on aarch64 and x86_64), solves [#66](https://github.com/jguhlin/minimap2-rs/issues/66)
+
 ### 0.1.20 minimap2 2.28
 + Fix htslib errors. No update to -sys crate needed.
 
diff --git a/minimap2-sys/Cargo.toml b/minimap2-sys/Cargo.toml
index 17cf0eb..463a49e 100644
--- a/minimap2-sys/Cargo.toml
+++ b/minimap2-sys/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "minimap2-sys"
-version = "0.1.19+minimap2.2.28"
+version = "0.1.20+minimap2.2.28"
 edition = "2021"
 links = "libminimap2"
 authors = ["Joseph Guhlin <joseph.guhlin@gmail.com>"]
diff --git a/minimap2-sys/README.md b/minimap2-sys/README.md
index cea5ea7..c066a37 100644
--- a/minimap2-sys/README.md
+++ b/minimap2-sys/README.md
@@ -1,8 +1,8 @@
 # System Bindings for libminimap2
-Use this if you need lower-level bindings for minimap2. Also works with mm2-fast.
+Use this if you need lower-level bindings for minimap2.
 
 # Minimap2 Version
-Sync'd to minimap2 2.27
+Minimap2 2.28
 
 ## Breaking Changes
 ### 0.0.18
@@ -10,14 +10,21 @@ mm2-fast and minimap2 have diverged. At this point mm2-fast is no longer support
 
 ## Features 
 * vendored - Regenerate the bindings from the vendored minimap2 source. Requires llvm installed. Useful to update the bindings to a different version of minimap2.
-* mm2-fast - Use [mm2-fast](https://github.com/bwa-mem2/mm2-fast) as the backend rather than minimap2 itself.
 * simde - Enable simde support (SIMD-everywhere)
 * sse - Enable some sse bindings
+* zlib-ng - Use zlib-ng
+* static - Static compilation
 
 ## TODO
 * Can we decouple from pthread? This would allow Windows and (possibly) WASM compilation.
 
 ## Changelog
+### 0.1.20 minimap2.2.28
+* Move Drop impl to -sys crate
+
+### 0.1.19 minimap2.2.28
+* Update to minimap2 2.28
+
 ### 0.1.18 minimap2.2.27
 * Regenerated bindings
 * mm2-fast and minimap2 have diverged
diff --git a/minimap2-sys/src/bindings.rs b/minimap2-sys/src/bindings.rs
index ed56e1f..a722564 100644
--- a/minimap2-sys/src/bindings.rs
+++ b/minimap2-sys/src/bindings.rs
@@ -4686,7 +4686,7 @@ fn bindgen_test_layout_mm_idx_seq_t() {
     );
 }
 #[repr(C)]
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Clone)]
 pub struct mm_idx_t {
     pub b: i32,
     pub w: i32,
diff --git a/minimap2-sys/src/lib.rs b/minimap2-sys/src/lib.rs
index e5640ec..2540154 100644
--- a/minimap2-sys/src/lib.rs
+++ b/minimap2-sys/src/lib.rs
@@ -12,6 +12,13 @@ unsafe impl Send for mm_idx_t {}
 unsafe impl Send for mm_idx_reader_t {}
 unsafe impl Send for mm_mapopt_t {}
 
+impl Drop for mm_idx_t {
+    fn drop(&mut self) {
+        unsafe { mm_idx_destroy(self) };
+    }
+}
+
+
 use std::mem::MaybeUninit;
 
 impl Default for mm_mapopt_t {
diff --git a/src/htslib.rs b/src/htslib.rs
index eda99f4..a4fbf53 100644
--- a/src/htslib.rs
+++ b/src/htslib.rs
@@ -50,6 +50,7 @@ use rust_htslib::bam::{Header, HeaderView, Record};
 use std::ffi::{CStr, CString};
 use std::mem::MaybeUninit;
 use std::ptr;
+use std::sync::Arc;
 
 /// A wrapper around mm_bseq1_t
 #[derive(Debug)]
@@ -164,7 +165,7 @@ impl Aligner {
 
             let mm_reg = MaybeUninit::new(unsafe {
                 mm_ffi::mm_map(
-                    self.idx.unwrap() as *const mm_ffi::mm_idx_t,
+                    &**self.idx.as_ref().unwrap().as_ref() as *const mm_ffi::mm_idx_t,
                     query.inner.l_seq,
                     query.inner.seq as *const libc::c_char,
                     &mut n_regs,
@@ -191,7 +192,7 @@ impl Aligner {
                     // TODO: use mm_write_sam3 t do the writing so that we can pass the map_opt flags
                     mm_ffi::mm_write_sam(
                         result.as_mut_ptr(),
-                        self.idx.unwrap() as *const mm_ffi::mm_idx_t,
+                        &**self.idx.as_ref().unwrap().as_ref() as *const mm_ffi::mm_idx_t,
                         &query.inner as *const mm_ffi::mm_bseq1_t,
                         const_ptr,
                         n_regs,
@@ -304,19 +305,20 @@ pub struct SeqMetaData {
 
 #[derive(Debug)]
 pub struct MMIndex {
-    pub inner: mm_ffi::mm_idx_t,
+    pub inner: Arc<*mut mm_ffi::mm_idx_t>,
 }
 
 impl MMIndex {
     pub fn n_seq(&self) -> u32 {
-        self.inner.n_seq
+        unsafe { (**self.inner).n_seq }        
     }
 
     pub fn seqs(&self) -> Vec<SeqMetaData> {
         let mut seqs: Vec<SeqMetaData> = Vec::with_capacity(self.n_seq() as usize);
         for i in 0..self.n_seq() {
-            let _seq = unsafe { *(self.inner.seq).offset(i as isize) };
-            let c_str = unsafe { ffi::CStr::from_ptr(_seq.name) };
+            let _seq = unsafe { *(**self.inner).seq.offset(i as isize) };
+            // let c_str = unsafe { ffi::CStr::from_ptr(_seq.name) };
+            let c_str = unsafe { CStr::from_ptr(_seq.name) };
             let rust_str = c_str.to_str().unwrap().to_string();
             seqs.push(SeqMetaData {
                 name: rust_str,
@@ -344,7 +346,7 @@ impl From<&Aligner> for MMIndex {
     fn from(aligner: &Aligner) -> Self {
         MMIndex {
             // inner: aligner.idx.unwrap(),
-            inner: unsafe { **aligner.idx.as_ref().unwrap() },
+            inner: std::sync::Arc::clone(&aligner.idx.as_ref().unwrap())
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index cc0729f..cb6b637 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,4 +1,4 @@
-//! API providing a rusty interface to minimap2 or mm2-fast libraries.
+//! API providing a rusty interface to minimap2
 //!
 //! This library supports statically linking and compiling minimap2 directly, no separate install is required.
 //!
@@ -12,10 +12,26 @@
 //! # Crate Features
 //! This crate has multiple create features available.
 //! * map-file - Enables the ability to map a file directly to a reference. Enabled by deafult
-//! * mm2-fast - Uses the mm2-fast library instead of standard minimap2
 //! * htslib - Provides an interface to minimap2 that returns rust_htslib::Records
 //! * simde - Enables SIMD Everywhere library in minimap2
-//! * sse - Enables the use of SSE instructions
+//! * zlib-ng - Enables the use of zlib-ng for faster compression
+//! * curl - Enables curl for htslib
+//! * static - Builds minimap2 as a static library
+//! * sse2only - Builds minimap2 with only SSE2 support
+//! 
+//! ## Previously Supported Features
+//! * mm2-fast - Uses the mm2-fast library instead of standard minimap2
+//! 
+//! If needed, this can be re-enabled.
+//! 
+//! # Compile-time options
+//! I recommend the following:
+//! ```toml
+//! [profile.release]
+//! opt-level = 3
+//! lto = "fat"
+//! codegen-units  = 1
+//! ```
 //!
 //! # Examples
 //! ## Mapping a file to a reference
@@ -57,6 +73,8 @@ use std::os::unix::ffi::OsStrExt;
 use libc::c_void;
 use minimap2_sys::*;
 
+pub use minimap2_sys as ffi;
+
 #[cfg(feature = "map-file")]
 use needletail::parse_fastx_file;
 
@@ -278,10 +296,10 @@ pub struct Aligner {
     pub threads: usize,
 
     /// Index created by minimap2
-    pub idx: Option<*mut mm_idx_t>,
+    pub idx: Option<Arc<*mut mm_idx_t>>,
 
     /// Index reader created by minimap2
-    pub idx_reader: Option<mm_idx_reader_t>,
+    pub idx_reader: Option<Arc<mm_idx_reader_t>>,
 
     /// Whether to add soft clipping to CIGAR result
     pub cigar_clipping: bool,
@@ -319,6 +337,34 @@ impl Aligner {
 
         aligner
     }
+
+    /// Returns the number of sequences in the index
+    pub fn n_seq(&self) -> u32 {
+        unsafe {
+            let idx = Arc::as_ptr(self.idx.as_ref().unwrap());
+            let idx: *const mm_idx_t = *idx;
+            (*idx).n_seq as u32
+        }
+    }
+
+    /// Get sequences direct from the index
+    /// 
+    /// Returns a reference to the sequence at the given index
+    /// Remainds valid as long as the aligner is valid
+    pub fn get_seq<'aln>(&'aln self, i: usize) -> Option<&'aln mm_idx_seq_t> {
+        unsafe {
+            let idx = Arc::as_ptr(self.idx.as_ref().unwrap());
+            let idx: *const mm_idx_t = *idx;
+            // todo, should this be > or >=
+            if i > self.n_seq() as usize {
+                return None;
+            }
+            let seq = (*idx).seq;
+            let seq = seq.offset(i as isize);
+            let seq = &*seq;
+            Some(seq)
+        }
+    }
 }
 
 impl Aligner {
@@ -679,7 +725,10 @@ impl Aligner {
             mm_idx_index_name(idx.assume_init());
         }
 
-        self.idx = Some(unsafe { idx.assume_init() });
+        let mm_idx = unsafe { idx.assume_init() };
+        self.idx = Some(Arc::new(mm_idx));
+        let mm_idx = unsafe { idx.assume_init() };
+        self.idx = Some(Arc::new(mm_idx));
 
         Ok(())
     }
@@ -786,7 +835,10 @@ impl Aligner {
             )
         });
 
-        self.idx = Some(unsafe { idx.assume_init() });
+        let mm_idx = unsafe { idx.assume_init() };
+        self.idx = Some(Arc::new(mm_idx));
+        let mm_idx = unsafe { idx.assume_init() };
+        self.idx = Some(Arc::new(mm_idx));
         self.mapopt.mid_occ = 1000;
 
         Ok(self)
@@ -871,7 +923,7 @@ impl Aligner {
 
             mm_reg = MaybeUninit::new(unsafe {
                 mm_map(
-                    self.idx.unwrap() as *const mm_idx_t,
+                    &**self.idx.as_ref().unwrap().as_ref() as *const mm_idx_t,
                     seq.len() as i32,
                     seq.as_ptr() as *const ::std::os::raw::c_char,
                     &mut n_regs,
@@ -880,6 +932,8 @@ impl Aligner {
                     qname,
                 )
             });
+
+
             let mut mappings = Vec::with_capacity(n_regs as usize);
 
             for i in 0..n_regs {
@@ -888,8 +942,14 @@ impl Aligner {
                     let const_ptr = reg_ptr as *const mm_reg1_t;
                     let reg: mm_reg1_t = *reg_ptr;
 
-                    let contig: *mut ::std::os::raw::c_char =
-                        (*((*(self.idx.unwrap())).seq.offset(reg.rid as isize))).name;
+                    let idx = Arc::as_ptr(self.idx.as_ref().unwrap());
+                    let contig = std::ffi::CStr::from_ptr(
+                        (*(**idx).seq.offset(reg.rid as isize)).name,
+                    );
+                    let idx = Arc::as_ptr(self.idx.as_ref().unwrap());
+                    let contig = std::ffi::CStr::from_ptr(
+                        (*(**idx).seq.offset(reg.rid as isize)).name,
+                    );
 
                     let is_primary = reg.parent == reg.id && (reg.sam_pri() > 0);
                     let is_supplementary = (reg.parent == reg.id) && (reg.sam_pri() == 0);
@@ -990,7 +1050,7 @@ impl Aligner {
                                     km,
                                     &mut cs_string,
                                     &mut m_cs_string,
-                                    self.idx.unwrap() as *const mm_idx_t,
+                                    &**self.idx.as_ref().unwrap().as_ref() as *const mm_idx_t,
                                     const_ptr,
                                     seq.as_ptr() as *const libc::c_char,
                                     true.into(),
@@ -1009,7 +1069,7 @@ impl Aligner {
                                     km,
                                     &mut cs_string,
                                     &mut m_cs_string,
-                                    self.idx.unwrap() as *const mm_idx_t,
+                                    &**self.idx.as_ref().unwrap().as_ref() as *const mm_idx_t,
                                     const_ptr,
                                     seq.as_ptr() as *const libc::c_char,
                                 );
@@ -1040,16 +1100,19 @@ impl Aligner {
                     };
 
                     let target_name_arc = Arc::new(
-                        std::ffi::CStr::from_ptr(contig)
+                        std::ffi::CStr::from_ptr(contig.as_ptr())
                             .to_str()
                             .unwrap()
                             .to_string(),
                     );
 
+                    let target_len = (*(**idx).seq.offset(reg.rid as isize)).len as i32;
+
+                    let target_len = (*(**idx).seq.offset(reg.rid as isize)).len as i32;
+
                     mappings.push(Mapping {
                         target_name: Some(Arc::clone(&target_name_arc)),
-                        target_len: (*((*(self.idx.unwrap())).seq.offset(reg.rid as isize))).len
-                            as i32,
+                        target_len,
                         target_start: reg.rs,
                         target_end: reg.re,
                         query_name: query_name_arc.clone(),
@@ -1153,20 +1216,6 @@ mod send {
     unsafe impl Send for Aligner {}
 }
 
-/* TODO: This stopped working when we switched to not storing raw pointers but the structs themselves
-/ Since Rust is now handling the structs, I think memory gets freed that way, maybe this is no longer
-/ necessary?
-/ TODO: Test for memory leaks
-*/
-impl Drop for Aligner {
-    fn drop(&mut self) {
-        if self.idx.is_some() {
-            let idx = self.idx.take().unwrap();
-            unsafe { mm_idx_destroy(idx) };
-        }
-    }
-}
-
 #[derive(PartialEq, Eq)]
 pub enum FileFormat {
     FASTA,
@@ -1827,20 +1876,20 @@ mod tests {
                 Err("File does not exist")
             );
 
-            if let Err("File is empty") = Aligner::builder().with_index("test_data/empty.fa", None)
+            if let Err("Index File is empty") = Aligner::builder().with_index("test_data/empty.fa", None)
             {
                 println!("File is empty - Success");
             } else {
                 panic!("File is empty error not thrown");
             }
 
-            if let Err("Invalid Path") = Aligner::builder().with_index("\0invalid_\0path\0", None) {
+            if let Err("Invalid Path for Index") = Aligner::builder().with_index("\0invalid_\0path\0", None) {
                 println!("Invalid Path - Success");
             } else {
                 panic!("Invalid Path error not thrown");
             }
 
-            if let Err("Invalid Output") =
+            if let Err("Invalid Output for Index") =
                 Aligner::builder().with_index("test_data/MT-human.fa", Some("test\0test"))
             {
                 println!("Invalid output - Success");
@@ -1935,4 +1984,141 @@ mod tests {
             ..sr
         };
     }
+
+    #[test]
+    fn double_free_index_test() {
+        // Create a new aligner
+        println!("Creating aligner");
+        let aligner = Aligner::builder().map_ont().with_index("yeast_ref.mmi", None).unwrap();
+        println!("Aligner created");
+
+        // Perform a test mapping to ensure the index is loaded and all
+        let mappings = aligner.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+        assert!(mappings.len() > 0);
+
+        println!("Going into threads");
+
+        // Spawn two threads using thread scoped aligners, clone aligner
+        std::thread::scope(|s| {
+            let aligner_ = aligner.clone();
+
+            // Confirm that aligner_ idx points to the same memory as aligner idx arc
+            assert_eq!(Arc::as_ptr(aligner.idx.as_ref().unwrap()), Arc::as_ptr(aligner_.idx.as_ref().unwrap()));
+
+            // Confirm we have a strong count of 2
+            assert_eq!(Arc::strong_count(&aligner.idx.as_ref().unwrap()), 2);
+
+            let jh0 = s.spawn(move || {
+                let mappings = aligner_.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+                assert!(mappings.len() > 0);
+                // Sleep 100ms
+                std::thread::sleep(std::time::Duration::from_millis(100));
+            });
+
+            let aligner_ = aligner.clone();
+            let jh1 = s.spawn(move || {
+                std::thread::sleep(std::time::Duration::from_millis(200));
+                let mappings = aligner_.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+                assert!(mappings.len() > 0);
+                // Sleep 100ms
+                std::thread::sleep(std::time::Duration::from_millis(100));
+            });
+
+            jh0.join().unwrap();
+            jh1.join().unwrap();
+        });
+
+        println!("Past the first one");
+
+        // Create a new aligner
+        let aligner = Aligner::builder().map_ont().with_index("yeast_ref.mmi", None).unwrap();
+
+        // Perform a test mapping to ensure the index is loaded and all
+        let mappings = aligner.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+        assert!(mappings.len() > 0);
+
+        // Spawn two threads using thread scoped aligners, clone aligner
+        std::thread::scope(|s| {
+            let aligner0 = aligner.clone();
+            let aligner1 = aligner.clone();
+            
+            // Force drop logic
+            drop(aligner);
+
+            let jh0 = s.spawn(move || {
+                println!("First thread");
+                let mappings = aligner0.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+                assert!(mappings.len() > 0);
+                // Sleep 100ms
+                std::thread::sleep(std::time::Duration::from_millis(100));
+                println!("First thread done");
+            });
+
+            // Join, to force drop logic from external thread
+            jh0.join().unwrap();
+
+            let jh1 = s.spawn(move || {
+                println!("Second thread");
+                std::thread::sleep(std::time::Duration::from_millis(200));
+                let mappings = aligner1.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+                assert!(mappings.len() > 0);
+                // Sleep 100ms
+                std::thread::sleep(std::time::Duration::from_millis(100));
+                assert!(Arc::strong_count(&aligner1.idx.as_ref().unwrap()) == 1);
+                println!("Second thread done");                
+            });
+
+            jh1.join().unwrap();
+        });
+
+        println!("Moving to the third test");
+
+        // Finally with no test mapping
+        // Create a new aligner
+        let aligner = Aligner::builder().map_ont().with_index("yeast_ref.mmi", None).unwrap();
+
+        // Spawn two threads using thread scoped aligners, clone aligner
+        std::thread::scope(|s| {
+            let aligner0 = aligner.clone();
+            let aligner1 = aligner.clone();
+            
+            // Force drop logic
+            drop(aligner);
+
+            let jh0 = s.spawn(move || {
+                println!("First thread - No mapping");
+                let mappings = aligner0.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+                assert!(mappings.len() > 0);
+                // Sleep 100ms
+                std::thread::sleep(std::time::Duration::from_millis(100));
+            });
+
+            // Join, to force drop logic from external thread
+            jh0.join().unwrap();
+
+            let jh1 = s.spawn(move || {
+                println!("Second thread - No mapping");
+                std::thread::sleep(std::time::Duration::from_millis(200));
+                let mappings = aligner1.map("ACGGTAGAGAGGAAGAAGAAGGAATAGCGGACTTGTGTATTTTATCGTCATTCGTGGTTATCATATAGTTTATTGATTTGAAGACTACGTAAGTAATTTGAGGACTGATTAAAATTTTCTTTTTTAGCTTAGAGTCAATTAAAGAGGGCAAAATTTTCTCAAAAGACCATGGTGCATATGACGATAGCTTTAGTAGTATGGATTGGGCTCTTCTTTCATGGATGTTATTCAGAAGGAGTGATATATCGAGGTGTTTGAAACACCAGCGACACCAGAAGGCTGTGGATGTTAAATCGTAGAACCTATAGACGAGTTCTAAAATATACTTTGGGGTTTTCAGCGATGCAAAA".as_bytes(), false, false, None, None, Some("Sample Query")).unwrap();
+                assert!(mappings.len() > 0);
+                // Sleep 100ms
+                std::thread::sleep(std::time::Duration::from_millis(100));
+            });
+
+            jh1.join().unwrap();
+        });
+    }
+
+    // Test aligner cloning for flag permanence
+    #[test]
+    fn aligner_cloning_flags() {
+        let aligner = Aligner::builder().map_ont().with_cigar().with_index("yeast_ref.mmi", None).unwrap();
+        // Confirm with_cigar is set
+        // self.mapopt.flag |= MM_F_CIGAR as i64;
+        assert_eq!(aligner.mapopt.flag & MM_F_CIGAR as i64, MM_F_CIGAR as i64);
+
+        // Clone aligner
+        let aligner_clone = aligner.clone();
+        assert_eq!(aligner_clone.mapopt.flag & MM_F_CIGAR as i64, MM_F_CIGAR as i64);
+    }
 }


Test patch:
diff --git a/.github/workflows/build-test-rust-minimap2-sys.yml b/.github/workflows/build-test-rust-minimap2-sys.yml
index c65727f..24f7b3d 100644
--- a/.github/workflows/build-test-rust-minimap2-sys.yml
+++ b/.github/workflows/build-test-rust-minimap2-sys.yml
@@ -49,17 +49,15 @@ jobs:
     - run: cd minimap2-sys
     - name: Run minimap2-sys tests on Android aarch64
       run: cross test --target aarch64-linux-android
-    - name: Run minimap2-sys tests on Android armv7
-      run: cross test --target armv7-linux-androideabi
-    - name: Run minimap2-sys tests on Android i686
-      run: cross test --target i686-linux-android
     - name: Run minimap2-sys tests on Android x86_64
       run: cross test --target x86_64-linux-android
 
   test-macos-13:
     runs-on: macos-13
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
+      with:
+        submodules: 'recursive'
     - run: cd minimap2-sys
     - name: Run minimap2-sys tests
       run: cargo test
diff --git a/.github/workflows/build-test-rust-minimap2.yaml b/.github/workflows/build-test-rust-minimap2.yaml
index 2b606a9..84bc618 100644
--- a/.github/workflows/build-test-rust-minimap2.yaml
+++ b/.github/workflows/build-test-rust-minimap2.yaml
@@ -55,3 +55,17 @@ jobs:
       run: cargo test --features htslib
     - name: Run tests simde
       run: cargo test --features simde
+
+  test-android:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        submodules: 'recursive'
+    - name: Install Rust Cross
+      run: rustup target add aarch64-linux-android armv7-linux-androideabi i686-linux-android x86_64-linux-android
+    - run: cargo install cross --git https://github.com/cross-rs/cross
+    - name: Run minimap2 tests on Android aarch64
+      run: cross test --target aarch64-linux-android
+    - name: Run minimap2 tests on Android x86_64
+      run: cross test --target x86_64-linux-android
\ No newline at end of file