Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider dropping Send requirement on generic Read param to FastxReader #95

Open
jchorl opened this issue Jan 23, 2025 · 1 comment
Open

Comments

@jchorl
Copy link
Contributor

jchorl commented Jan 23, 2025

Howdy!

I was playing around with some wasm stuff using needletail. One of the main challenges is that most js_sys js objects are not Send.

Consider this example:

use needletail::parse_fastx_reader;
use wasm_bindgen::prelude::*;
use web_sys::FileReaderSync;

struct MyReader {
    reader: FileReaderSync,
}

impl std::io::Read for MyReader {
    fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> { Ok(0) }
}

#[wasm_bindgen]
pub fn some_func(reader: FileReaderSync) {
    parse_fastx_reader(MyReader{ reader });
}

Running wasm-pack build --target web:

error[E0277]: `*mut u8` cannot be sent between threads safely                  
    --> src/lib.rs:17:24                                                                                                                                       
     |                                                                         
17   |     parse_fastx_reader(MyReader{ reader });   
     |     ------------------ ^^^^^^^^^^^^^^^^^^ `*mut u8` cannot be sent between threads safely                                                               
     |     |                                                                   
     |     required by a bound introduced by this call                         
     |                                 
     = help: within `MyReader`, the trait `Send` is not implemented for `*mut u8`, which is required by `MyReader: Send`

...

note: required by a bound in `parse_fastx_reader`
    --> /home/j/third-party/needletail/src/parser/mod.rs:85:50
     |
85   | pub fn parse_fastx_reader<'a, R: 'a + io::Read + Send>(
     |                                                  ^^^^ required by this bound in `parse_fastx_reader`

However, it seems Send isn't strictly required. Dropping Send from the FastxReader here:

pub trait FastxReader: Send {

And dropping it from both impls, the fastq and fasta reader, fixes compilation.

I'm sure I could wrap the reader in a mutex to make it Send but hammering a mutex (for lots of reads) introduces a lot of overhead.

From reading, it seems like Send is a nicety but even without the supertrait, if a caller passes in a Read that is also Send, the returned FastxReader would still be Send (but it's not always guaranteed to be). I'm not positive I have this correct.

So anyway, I wanted to see if you'd be open to removing Send from the fastx reader, and if you know of anything that could break in that case.

Thanks!

@luizirber
Copy link
Contributor

I got needletail to work in wasm with an, err, cheat:
https://github.com/sourmash-bio/sourmash/blob/af534a9a04c4712e5d727832036dac73757b55ce/src/core/src/wasm.rs#L231-L233

I think this should be OK because using FileSyncReader implies it is running inside a web worker, and that is "single-threaded" and won't be send across "thread" boundaries. I might be wrong, but for now this has been working in the web component in https://branchwater.sourmash.bio and https://chill-filter.sourmash.bio without issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants