[IO] add handle copy function #120
Conversation
facebook-github-bot
added
the
CLA Signed
| */ | ||
| async function copy( | ||
| <<__AcceptDisposable>> ReadHandle $source, | ||
| <<__AcceptDisposable>> WriteHandle $target, |
There was a problem hiding this comment.
Personal preference, but isn't $source $destination a more natural pair?
| } | ||
|
|
||
| if (!$source->isEndOfFile()) { | ||
| await copy($source, $target, $chunk_size, $timeout_seconds); |
There was a problem hiding this comment.
I guess you implemented this via recursive call because of the DontAwaitInALoopLinter?
I guess we could violate this here, since this has the exact same performance characteristics.
There was a problem hiding this comment.
I guess you implemented this via recursive call because of the DontAwaitInALoopLinter?
yes
I guess we could violate this here, since this has the exact same performance characteristics.
why violate the linter rule if we can avoid it? 😛
There was a problem hiding this comment.
Suppress the linter.
why violate the linter rule if we can avoid it?
Because violating the linter rule doesn't consume stack frames, leading to fatals on large files ;)
| * If `$chunk_size` is `null`, there is no limit on the chunk size - this function will | ||
| * copy the content of $source all at once. | ||
| */ | ||
| async function copy( |
There was a problem hiding this comment.
To match the naming conventions, copyAsync() or is this handled by the gen rewriter?
There was a problem hiding this comment.
not sure about this. other functions in File\ don't use the _async suffix.
There was a problem hiding this comment.
it should be copy_async
Other functions in File\ do not return Awaitables, so do not have the suffix (if that's not the case, it's a bug that needs fixing)
|
This is going to need some reworking with the changes to read api and Upcoming Corresponding changes to write api Additionally, there’ll be the scope for some optimization , e.g if real file to socket, use sendfile() |
There was a problem hiding this comment.
Also, sorry, I will not be merging this - or anything similar - until the APIs are stable after moving over to OS\FileDescriptor; this is not about the quality of the code, just timing. It definitely should be here in some form, but it is not a good time for API expansion at present.
HSL IO is still truly experimental, and unsuitable for use in any more other than standalone CLI - all error reporting is broken in HTTP and CLI server modes (which is fixed by the move to OS\FileDescriptor)
We're also restricting internal callers at Facebook, for both of the same reasons
|
The previous blockers are gone - closing at it would now need to be against the main HHVM repo. |
|
will open a PR against hhvm :) |
closes #44