Closed ikaronen-relex closed 1 year ago
I think at a minimum JRuby should provide some obvious signal that the file is not seekable (perhaps &.seekable? == false) if that becomes the case we can feature detect and switch implementation.
As what to do if JRuby is not going to change anything, I would recommend some sort of magic include that switches the implementation to a completely in-memory implementation if you opt for it (eg: require "mini_mime/in_memory"
)
So we leave RandomAccessDb as is and just implement a new InMemoryDb
that follows the same interface, then you could opt upfront for InMemoryDb over RandomAccessDb.
cc @headius
Worse, due to a questionable decision by the JRuby devs, attempting to seek them doesn't actually raise an exception but fails silently (and, AFAICT, also causes some internal buffers to be dumped so that the next readline ends up starting from the middle of a line).
We should fix the buffer dumping, probably. I would guess it was just an oversight... we flush buffers before seek and then don't actually seek for these unseekable resources, but we can't go back to where we were in the buffer.
I think at a minimum JRuby should provide some obvious signal that the file is not seekable (perhaps &.seekable? == false)
There is no IO#seekable?
in Ruby though. I agree, it would be a good idea, but we don't unilaterally add APIs like this.
We are always happy to fix issues in JRuby in a logical way, if you can summarize what you think should be fixed. If it's just repairing the buffer flush, that should be pretty easy.
We are always happy to fix issues in JRuby in a logical way, if you can summarize what you think should be fixed. If it's just repairing the buffer flush, that should be pretty easy.
@headius I agree that the buffer flush should certainly be fixed. Beyond that, though, I personally also think that a non-trivial seek on a non-seekable file should always raise an exception — where non-trivial means anything that isn't provably a no-op, such as f.seek(0, IO::SEEK_CUR)
. Such a failed non-trivial seek means that the program has requested a state change that the runtime cannot provide, and assumes it to have succeeded (since IO#seek
provides no non-exceptional way to indicate failure), thus placing the system in an unexpected and possibly inconsistent state. IMO that's something that pretty much always should result in an exception.
But that's all tangential to the issue at hand: even if my suggestion above was implemented, it would just make MiniMime fail sooner and in a more obvious way when run from a .jar bundle. I can file a separate issue for JRuby if you'd like, but I don't think further in-depth discussion about changing the JRuby IO#seek
behavior belongs here.
@SamSaffron Meanwhile, I'll try to provide a PR for MiniMime soon, probably tomorrow. AFAICT, the method I suggested earlier for detecting seekability (seek, rescue, check pos) should work reliably with or without any changes to JRuby's behavior. (To be safe, we do need to reopen the file if seeking fails, but that's easy enough to do in this case.)
I can file a separate issue for JRuby if you'd like
I missed this when you first posted, @ikaronen-relex, but it's a good idea if you're still out there :-)
@headius, sure, I can do that. Thanks for the reminder! 😃
Ps. I have a suspicion that the change in #50 doesn't actually fix the behavior when running from a bundled .jar file, since the JRuby implementation of pread
apparently silently ignores(!) the offset if fd.chFile
and fd.chNative
are both null (as they presumably are for streams opened from uri:classloader
URIs). I haven't tested that yet, but I'll create an internal ticket to remind me about it.
(FWIW, I also think that behavior is contrary to the POSIX spec, which says that "An attempt to perform a pread() on a file that is incapable of seeking results in an error." Arguably it also violates the Ruby stdlib documentation, which just references "the pread system call" and says that it "Raises SystemCallError on error". Anyway, I can also submit a fix for that, either in the same PR or in a separate one.)
We have a Rails-based JRuby application deployed as a bundled .jar file, in which we currently use ActionMailer (which uses the Mail gem, which use MiniMime) to send reports as e-mail attachments. We've had reports of weird random crashes causing these e-mails sometimes not to be sent, and we've managed to narrow down the source of these crashes to MiniMime, and specifically to the code in MiniMime::Db::RandomAccessDb.
Here's an example stack trace from a crash log:
(Yes, I know we're a few versions behind, but the relevant code in v1.0.2 looks the same as in master and I'm pretty sure the issue exists there too.)
Anyway, the weird crashes seem to happen because MiniMime::Db::RandomAccessDb.resolve tries to seek the DB file. But in a bundled app the DB files end up being
uri:classloader
resources, which are not seekable. Worse, due to a questionable decision by the JRuby devs, attempting to seek them doesn't actually raise an exception but fails silently (and, AFAICT, also causes some internal buffers to be dumped so that the nextreadline
ends up starting from the middle of a line).Anyway, while the weird silent seek failure is arguably a JRuby bug, even if it's fixed those files will never be seekable (since the underlying Java streams aren't). So this would still need to be fixed on MiniMime's side to make it work in a bundled JRuby app.
Off the top of my head, I can see a couple of possible solutions: