vionya / discord-rich-presence

A cross-platform Discord Rich Presence library written in Rust
MIT License
89 stars 16 forks source link

Windows IPC close method doing nothing #37

Open Bowarc opened 3 weeks ago

Bowarc commented 3 weeks ago

I maybe am missing something but it looks like the .close method in DiscordIpc's implementation on windows do nothing

Windows's implementation:

fn close(&mut self) -> Result<()> {
    let data = json!({});
    if self.send(data, 2).is_ok() {} // ?

    let socket = self.socket.as_mut().ok_or(Error::NotConnected)?;
    socket.flush()?;

    Ok(())
}

Unix's implementation

fn close(&mut self) -> Result<()> {
    let data = json!({});
    if self.send(data, 2).is_ok() {} // ?

    let socket = self.socket.as_mut().ok_or(Error::NotConnected)?;

    socket.flush()?;
    match socket.shutdown(Shutdown::Both) {
        Ok(()) => (),
        Err(_err) => (),
    };

    Ok(())
}

Has something been forgotten ?

vionya commented 2 weeks ago

so on windows the pipes are just normal file handles, and the File API doesnt have an explicit way to close a file (other than dropping it). i guess i figured that i didnt need to do anything to accomplish this when i first implemented windows (oops). i can add a call to std::mem::drop in the windows code, which should be explicit enough?

also, for the

    if self.send(data, 2).is_ok() {} // ?

lines, i don't really know what's going on there. they're removed in dev so i assume they aren't important

Bowarc commented 2 weeks ago

i can add a call to std::mem::drop in the windows code, which should be explicit enough?

If the code is doing what's expected, no need to modify it, maybe a comment would clarify this for any future reader ?

That said, now that i look at it, the file handle is not dropped, you only drop the reference you created with self.socket.as_mut().ok_or(Error::NotConnected)?;

To actually drop the File don't you need to .take self.socket and then drop it ?

Maybe im missing something

vionya commented 2 weeks ago

yea I think you're right, I didn't properly drop the file when I first wrote this lol. I'll update the code to properly drop the file