a-h / gemini

MIT License
45 stars 3 forks source link

Serve mp3 #9

Closed shrimpbaron closed 3 years ago

shrimpbaron commented 3 years ago

Thankyou for your excellent work, I'm attempting serve mp3 files with little success. Clients return a timeout message. Have I failed to set the server up correctly maybe ?

a-h commented 3 years ago

Hi @shrimpbaron, my first thought is that I've set the default server read and write timeouts really short, at 5 and 10 seconds.

That's likely the problem. Sorry, but I've not made it configurable as a setting from the server yet - I'll do that, but not likely until the weekend. If you clone the repo, you'd just need to change these lines and build the server to test it:

https://github.com/a-h/gemini/blob/057d92316b047aa51fa0d753670fa86d1c4d5cf9/server.go#L102

To build the server:

cd cmd
go build -o gemini

The server will then be output as gemini.

I haven't been serving up files that are as big are your MP3s probably are, hence not encountering the issue. The slowest request on my server was still less than 200ms in the last few days.

Screen Shot 2021-03-11 at 19 04 41
shrimpbaron commented 3 years ago

Ok, I will await your update (no rush). Another thing to consider - I tried a much smaller sound file and although the client (Kristal) received it, the file showed as garbled text. Castor just crashes with any sized file. Again the fault may be mine, I am new to geminispace. Many thanks once more.

My page is here if you wish to test.

gemini://shrimpbaron.cyou/sounds/sounds.gmi

a-h commented 3 years ago

I've done a bit more investigation, and I haven't been able to reproduce the issue locally yet. I've added some extra unit tests that include serving up an mp3 that are working fine, and I've also got an integration test localy.

I was able to download the MP3 no problem and the hashes match.

Next, I'll try and reproduce the issue reliably by slowing down the download and increasing the file sizes substantially.

a-h commented 3 years ago

I've added a configurable read and write timeout to the server so that you can choose values that make sense for you.

I've also improved the logs so that it's possible to see how many bytes were transferred.

{"code":"20","handlerType":"github.com/a-h/gemini","len":681820189,"lenBody":681820157,"lenHeader":32,"level":"INFO","ms":3107,"msg":"gemini: response","path":"/cubestacker.7z","time":"2021-03-15T21:25:06Z","url":"gemini://a-h.gemini/cubestacker.7z","us":3107523}

The default 10 second timeout may be too small for a larger download. The timeouts are relatively short because large timeouts allow for easy slow loris style attacks (in Gemini, attackers would just open loads of connection, and read very slowly from the server rather than the equivalent in HTTP which involves HTTP headers). AFAIK, there's currently no way for the client to tell that not all of the data was transferred, because Gemini doesn't have the equivalent of a HTTP Content-Length header.

You can increase the timeouts with the new options:

  -readTimeout duration
        Set the duration, e.g. 1m or 5s. (default 5s)
  -writeTimeout duration
        Set the duration, e.g. 1m or 5s. (default 10s)

I was thinking about enabling a sliding window for timeouts. For example, letting the user stay on for longer if they read a minimum of data in a period, but that's a bigger change.

Bear in mind that the client will have a timeout too. If the client times out first, the server will now log an error showing a write timeout.

a-h commented 3 years ago

For the garbled response, it could be down to the client not reading the MIME type of the Gemini response. This server correctly sets the audio/mpeg MIME type in the meta field of the response for MP3 files, so the client should not interpret the response as text, but should handle it as binary data instead.

Although my browser (https://github.com/a-h/min) does this correctly, the CLI request tool I use for testing things didn't... I fixed it in https://github.com/a-h/gemini/commit/e645cd91608671b9da412d308f9a6b03e92d7bc0 so if you're a programmer, that might help explain what's going on.

I use the test program to do things like download files and get a hash to verify that the data has transferred correctly.

gemini request --allowBinary --insecure gemini://a-h.gemini/cubestacker.7z | shasum -a 256

I've pushed a new release with the changes anyway.

https://github.com/a-h/gemini/releases/tag/v0.0.61

If you could give the new server a try with higher timeouts and see if that solves your problem for now, that would be great. 😀

shrimpbaron commented 3 years ago

Hello a-h, great work on the new release, I have widened the timeout window and can now fully download my rather large mp3 file. However both Kristall and Castor browsers still read the file as text. I wondered if there was an issue with my file so I have tried serving with the Agate server and that works fine. I can either switch to Agate or continue to test for you. How would you like me to proceed ? Many thanks again.

a-h commented 3 years ago

Thanks for testing that out!

I've tested the file download with Bombadillo and Amfora from this server, which worked fine.

I couldn't run Kristall myself, due to problems with the Nix Mac version.

However, I found that Castor just tries to open a text file for me, so I tried out the download with the Agate server to see whether there was some difference between my code and what Agate does, but Castor did the same thing with Agate for me. Although it pops open a text file, it did actually download the file perfectly well.

Looking through the code in Castor, it just creates a temp file and uses the open command, but the temp file it creates doesn't have a file extension, so the OS has no idea what to do with it...

pub fn download(content: Vec<u8>) {
    let path = write_tmp_file(content);
    println!("path: {}", path.display())
    open::that(path).unwrap();
}

My browser uses this function to set the extension on the downloaded temp file to avoid it. A similar change in Castor is needed.

func getFileNameFromURL(u *url.URL, mimeType string) string {
    fileName := path.Base(u.String())
    if fileName == "." {
        fileName = fmt.Sprintf("download_%d", time.Now().Unix())
    }
    if path.Ext(fileName) == "" {
        extensions, _ := mime.ExtensionsByType(mimeType)
        if len(extensions) > 0 {
            fileName += extensions[0]
        }
    }
    return fileName
}
shrimpbaron commented 3 years ago

Ok, ty.