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

Rewrite file systems to use ByteChannels #575

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SquidDev
Copy link
Contributor

@SquidDev SquidDev commented Sep 21, 2018

This replaces the existing IMount openFor* method with openChannelFor* ones, which return an appropriate byte channel instead. I've put a little more detail in the commit message about the implications of this, but there's a couple of user-facing benefits:

  • Add support for seekable handles. This works for any main filesystem or HTTP handle opened in binary mode.
  • Rewrite the io library to more accurately emulate PUC Lua's implementation. This passes a large portion of the the test suite - the remaining issues mainly involve their use of os.date and setvbuf, neither of which are implemented.

This replaces the existing IMount openFor* method with openChannelFor*
ones, which return an appropriate byte channel instead.

As channels are not correctly closed when GCed, we introduce a
FileSystemWrapper. We store a weak reference to this, and when it is
GCed or the file closed, we will remove it from our "open file" set and
ensure any underlying buffers are closed.

While this change may seem a little odd, it does introduce some
benefits:

 - We can replace JarMount with a more general FileSystemMount. This
   does assume a read-only file system, but could technically be used
   for other sources.

 - Add support for seekable (binary) handles. We can now look for
   instances of SeekableByteChannel and dynamically add it. This works
   for all binary filesystem and HTTP streams.

 - Rewrite the io library to more accurately emulate PUC Lua's
   implementation. We do not correctly implement some elements (most
   noticably "*n", but it's a definite improvement.
@SquidDev SquidDev force-pushed the feature/file-seeking branch from d6c5631 to 518eefb Compare September 26, 2018 09:01
 - Rename openStreamFor* methods to more accurate openChannelFor*
 - Fix ArrayByteChannel having an incorrect .position() implementation
 - Fix stdin not being considered a "readable" input
 - Return an unsigned byte rather than a signed one for no-args .read()
@SquidDev
Copy link
Contributor Author

SquidDev commented Nov 3, 2018

Possibly worth noting that seekable binary handles do not work for Jar mounts - namely the rom and any treasure disks.

It would be possible to implement this by reading the entire contents of the file into memory and exposing it as an ArrayByteChannel, but I think it's worth discussing whether it's worth it.

ccserver pushed a commit to ccserver/ComputerCraft that referenced this pull request Sep 16, 2019
…file-seeking

Rewrite file systems to use ByteChannels
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

Successfully merging this pull request may close these issues.

1 participant