Make sure the DB file is seekable, use a StringIO wrapper if it's not (PR #38)

This (hopefully) fixes Seeking the DB file does not work in a bundled JRuby application, crashes randomly · Issue #37 · discourse/mini_mime · GitHub

What has been tested:

  • Still works when not bundled in a .jar, does not use StringIO wrapper.
  • Works and uses StringIO wrapper when patched so that the is_seekable check always fails.

What has not (yet) been tested:

  • Does it actually work with a uri:classloader resource in a bundled .jar, with seek failing silently?
  • Does it work with a non-seekable DB file in other cases, where seek might raise an exception?

While I’m 99% confident that the answer to both questions is yes, I’d still like to do a bit more testing. But I wanted to get this PR out for review before the weekend.

(Also, I’m not sure if there are any potential newline and/or UTF-8 conversion issues on some platforms that might require opening the DB files explicitly in binary mode. If there were, I suspect the existing code would already be buggy on such platforms, since it’s already seeking around in a file opened in text mode, but I’m still slightly worried and would like to see this tested better.)

GitHub

Looks like an ok approach. @jeremy how do you feel about this? adds two tell calls and one seek, seems reasonable in the big scheme.

Can we get away with a slightly less paranoid implementation? (seek 1 (if that fails) not seekable, if tell gives me anything that is not 1 then it is not seekable)

Tests are failing so something is off.

@headius FYI

Can we get away with a slightly less paranoid implementation? (seek 1 (if that fails) not seekable, if tell gives me anything that is not 1 then it is not seekable)

That was actually more or less what I started with, but I figured one extra tell wouldn’t hurt. AIUI tell is supposed to be a fast operation anyway. And since we’re reading the first line anyway, I could actually test a real backwards seek after read for basically free.

(Now, if I was actually paranoid, I’d readline again after the seek and check that I get the same line back as first time…)

Tests are failing so something is off.

:grimacing: Not much to go on in the test logs, but let me do some local testing and try to see what’s going wrong.

In local testing, I’m getting a failure in test_full_parity_with_mime_types for the extension .webmanifest, which should return “application/manifest+json” according to MIME::Types. However, rake rebuild_db fixes the failure. Should I just include the rebuilt DB in the PR?

.webmanifest was merged in DB updates 2021-08-01T10:14:51Z by github-actions[bot] · Pull Request #36 · discourse/mini_mime · GitHub, so you probably just need a rebase.

Agreed; reasonable.

On Sun, Aug 22, 2021 at 19:28 Sam @.***> wrote:

Looks like an ok approach. @jeremy https://github.com/jeremy how do you feel about this? adds two tell calls and one seek, seems reasonable in the big scheme.

Can we get away with a slightly less paranoid implementation? (seek 1 (if that fails) not seekable, if tell gives me anything that is not 1 then it is not seekable)

Tests are failing so something is off.

@headius https://github.com/headius FYI

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/discourse/mini_mime/pull/38#issuecomment-903395855, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAABR3TFJBDNUQFXLO54FLT6GW4PANCNFSM5CQKX3DA .