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

Fixes #35: fd{Read,Write} with ByteString payload #186

Closed
wants to merge 1 commit into from

Conversation

mmhat
Copy link
Contributor

@mmhat mmhat commented May 15, 2021

This commit addes two new functions: fdReadBytes and fdWriteBytes.
They are like fdRead and fdWrite respectively but instead of
reading/writing String they use ByteStrings.

Fixes #35

This commit addes two new functions: fdReadBytes and fdWriteBytes.
They are like fdRead and fdWrite respectively but instead of
reading/writing String they use ByteStrings.
@@ -1,7 +1,7 @@
{-# LANGUAGE CApiFFI #-}
{-# LANGUAGE NondecreasingIndentation #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE Safe #-}
{-# LANGUAGE Trustworthy #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this incidental cleanup, or "essential" to this PR? To be honest I've never taken the time to understand Safe, Trustworthy, ... So I don't know whether this change is correct or belongs in this PR (collateral cleanup is of course OK in some cases).

Copy link
Contributor Author

@mmhat mmhat Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is need; Otherwise we cannot safely import that module in System.Posix.IO.ByteString.

-- reached.
fdReadBytes :: Fd
-> ByteCount -- ^How many bytes to read
-> IO (ByteString, ByteCount) -- ^The bytes read, how many bytes were read.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ByteStrings already carry a length field, there's little point in returning it another copy wrapped in a tuple. This should use createUptoN and return just the ByteString.

Also, not all errors are invalid descriptor or EOF. On a non-blocking file descriptor, the error could be EAGAIN, and fdReadBuf will have thrown the corresponding IO Exception.
I am not sure that raising on EOF is the most useful result, returning an empty ByteString may be a cleaner choice, but perhaps we're too used to EOF exceptions in Haskell... :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not all errors are invalid descriptor or EOF. On a non-blocking file descriptor, the error could be EAGAIN, and fdReadBuf will have thrown the corresponding IO Exception.
I am not sure that raising on EOF is the most useful result, returning an empty ByteString may be a cleaner choice, but perhaps we're too used to EOF exceptions in Haskell... :-(

Not sure yet what my opinion is here; I tried to stick to the existing design (with all its bugs and quirks) here. If we decide to change the way how we handle EOF I suggest we do it in #219 and discuss it over there.

@hasufell
Copy link
Member

hasufell commented Jul 5, 2022

My opinion is that these shouldn't be new functions, but that the existing functions fdRead/fdWrite in System.Posix.IO.ByteString are buggy, since they don't use ByteString. That means they're in fact the same as their counterparts in System.Posix.IO:

That makes no sense.

So I think breaking API here is the right choice.

@mmhat
Copy link
Contributor Author

mmhat commented Jul 5, 2022

@hasufell I wholeheartedly aggree with you but wanted to be careful with backwards-incompatible changes. If no one objects I'll update this PR such that it fixes this bug and remove the new functions.

@hasufell
Copy link
Member

hasufell commented Jul 5, 2022

@hs-viktor opinions? Users who use the old functions can fix their code easily by importing the ones from System.Posix.IO instead. I think that's a reasonable migration strategy and can likely be automated for large codebases.

@hasufell
Copy link
Member

hasufell commented Jul 5, 2022

We could also add a test identical to https://github.com/haskell/unix/blob/master/tests/Posix014.hs

@mmhat
Copy link
Contributor Author

mmhat commented Jul 5, 2022

I opened another PR #219 implementing the fix like @hasufell suggested in #186 (comment).

@mmhat
Copy link
Contributor Author

mmhat commented Jul 5, 2022

We could also add a test identical to https://github.com/haskell/unix/blob/master/tests/Posix014.hs

@hasufell Will add one once we decide which PR will make it.

@Bodigrim
Copy link
Contributor

That means they're in fact the same as their counterparts in System.Posix.IO:

* https://hackage.haskell.org/package/unix-2.7.2.2/docs/System-Posix-IO.html#v:fdRead

* https://hackage.haskell.org/package/unix-2.7.2.2/docs/System-Posix-IO-ByteString.html#v:fdRead

That makes no sense.

On the surface level this is kinda expected, the contract is that System.Posix.ByteString represents all file paths and environment strings as ByteString instead of String, but other arguments or outputs remain unchanged. So functions which take neither file paths nor environment strings are indeed the same in both modules.

-- This module exports exactly the same API as "System.Posix", except
-- that all file paths and environment strings are represented by
-- 'ByteString' instead of 'String'. The "System.Posix" API
-- implicitly translates all file paths and environment strings using
-- the locale encoding, whereas this version of the API does no
-- encoding or decoding and works directly in terms of raw bytes.

However, the underlying rationale for the existence of System.Posix.ByteString is not performance per se, it's independence from locale encodings. So while it is expected to have some duplicates between System.Posix and System.Posix.ByteString and it is not a crime to be inefficient, locale-dependent decoding should not have place in System.Posix.ByteString.fdRead as it happily happens now. Since there is no way to return String without some kind of decoding (locale-driven or other), these functions should indeed return ByteString. I think a breaking change is justified here.

@hasufell
Copy link
Member

Dropped in favor of #219

@hasufell hasufell closed this Jul 16, 2022
@mmhat mmhat deleted the issue-35-bytestring-payload branch July 16, 2022 22:20
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.

Add convenient I/O operations using ByteString for payload
4 participants