FileHandle from SeekableByteChannel

[lhns]

Hi! I wanted to use https://github.com/robtimus/sftp-fs with the fs2 Files api. It turns out that some file systems don't implement a FileChannel so in this PR I propose a fallback solution to use SeekableByteChannel if necessary.
The FileHandle will not support features like locking in that case. Currently these methods just return F[Unit] but it might be better for them to throw an UnsupportedOperationException. Feedback would be welcome!

[armanbilge]

Instead of no-opping or erroring those methods, maybe we need to extract a parent interface? I.e. if all you have is a SeekableByteChannel is that really a FileHandle?

[lhns]

Yes that would probably be a better solution but requires greater changes. What about a SeekableBytesHandle as a parent trait of FileHandle? My main intent is to just be able to read and write files. I don't really need the other operations that FileHandle has.

[armanbilge]

I guess that's my other question. Do you actually need a FileHandle (e.g. to pass it to an API that expect one) or do you just want an effectual wrapper around SeekableByteChannel?

[lhns]

Well actually I just want to use readAll and writeAll. If WriteCursor and ReadCursor were a trait they could be implemented using SeekableByteChannel. Then I could catch the UnsupportedOperationException in Files.readCursor and Files.writeCursor and use the SeekableByteChannel implementation instead. Since that is not easily changable and ReadCursor and WriteCursor require a FileHandle I could construct the hacky FileHandle just for the cursors but it would still be accessible to the library user.

[armanbilge]

Thanks for the exposition. Hmm, that's frustrating. It seems that {Read,Write}Cursor don't really need those additional methods on FileHandle but its hard to fix that compatibly. Hmmm 🤔

[lhns]

I thought a bit about this but this currently seems to be the only way to get it working. I now made the unsupported methods throw an UnsupportedOperationException so instead of getting one when you open the FileHandle without this PR you now get one when using the unsupported methods which is probably equally unsafe. So its still somewhat of an hack but it gets reading and writing working with unsupported filesystems.

[mpilquist]

Throwing for the unsupported operations seems fine to me. Couple thoughts:

• If position is not settable, we can't even implement write correctly. The implementation of write in this PR is only safe for sequential writes.
• We should consider re-raising the original open error upon failure to open a byte channel.

I think the write issue is enough of a reason to not provide a WriteCursor for non-seekable channels.

[lhns]

Throwing for the unsupported operations seems fine to me. Couple thoughts:

• If position is not settable, we can't even implement write correctly. The implementation of write in this PR is only safe for sequential writes.
• We should consider re-raising the original open error upon failure to open a byte channel.

I think the write issue is enough of a reason to not provide a WriteCursor for non-seekable channels.

The implementation actually tries to set the position on non-sequential writes and will throw if the SeekableByteChannel doesn't support this doesn't it? This is a mutable operation though unlike in the FileChannel impl.

[mpilquist]

Whoops, you are right. I misread the guard. Objection withdrawn.On Oct 3, 2023, at 8:40 AM, Pierre Kisters ***@***.***> wrote: Throwing for the unsupported operations seems fine to me. Couple thoughts: If position is not settable, we can't even implement write correctly. The implementation of write in this PR is only safe for sequential writes. We should consider re-raising the original open error upon failure to open a byte channel. I think the write issue is enough of a reason to not provide a WriteCursor for non-seekable channels. The implementation actually tries to set the position on non-sequential writes and will throw if the SeekableByteChannel doesn't support this doesn't it? This is a mutable operation though unlike in the FileChannel impl. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

[lhns]

I changed the PR to re-raise the original exception that is thrown when creating the FileChannel. Is the mima error a real problem? As far as I can tell the hierarchy is sealed enough that it shouldn't cause any problems or am I missing something?

[armanbilge]

This one seems okay to filter, unfortunately MiMa doesn't recognized sealed traits I think.

[lhns]

I have added the mima exclusion, a test and updated the branch. The PR should now be ready to review.

[mpilquist]

Thanks!