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.

Sign up for GitHub

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

Jump to bottom

read larger chunks from sftp in stream wrapper #1452

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

read larger chunks from sftp in stream wrapper #1452

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97171c0
read larger chunks from sftp in stream wrapper
icewind1991 Feb 14, 2020
c819951
invalidate readahead buffer on write/seek/truncate
icewind1991 Feb 27, 2020
e1a1b11
make readahead length configurable
icewind1991 Feb 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 24 additions & 7 deletions phpseclib/Net/SFTP/Stream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ class Stream
*/
private $notification;

/**
* Buffer of data being read
*
* @var string
* @access public
*/
private $buffer = '';
Comment on lines +123 to +125
Copy link

Choose a reason for hiding this comment

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

Should this say @access private?


/**
* Registers this class as a URL wrapper.
*
Expand Down Expand Up @@ -315,16 +323,25 @@ private function _stream_read($count)
// return false;
//}

$result = $this->sftp->get($this->path, false, $this->pos, $count);
if (isset($this->notification) && is_callable($this->notification)) {
if ($result === false) {
call_user_func($this->notification, STREAM_NOTIFY_FAILURE, STREAM_NOTIFY_SEVERITY_ERR, $this->sftp->getLastSFTPError(), NET_SFTP_OPEN, 0, 0);
return 0;
if (strlen($this->buffer) < $count) {
// read 80k chunks from sftp even though php will only request 8k chunk
// this allows us to minimize the overhead of making sftp requests
$chunk = $this->sftp->get($this->path, false, $this->pos, 81920);
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the 81920 a parameter? I think there is a mechanism for that on stream construction.

Copy link
Member

Choose a reason for hiding this comment

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

I think what bantu is talking about is this:

<?php
require __DIR__ . '/vendor/autoload.php';

use phpseclib3\Net\SFTP\Stream;
use phpseclib3\Net\SFTP;

Stream::register();

$sftp = new SFTP('website.com');
$sftp->login('username', 'password');

$context = [
    'sftp' => ['sftp' => $sftp]
];
$context = stream_context_create($context);

$fp = fopen('sftp://dummy.com/home/vagrant/1mb', 'r', false, $context);
echo fread($fp, 10);

Other options are discussed at https://www.php.net/manual/en/wrappers.ssh2.php

Copy link
Member

Choose a reason for hiding this comment

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

Yes, "stream context" is what I was referring to.

if (isset($this->notification) && is_callable($this->notification)) {
if ($chunk === false) {
call_user_func($this->notification, STREAM_NOTIFY_FAILURE, STREAM_NOTIFY_SEVERITY_ERR, $this->sftp->getLastSFTPError(), NET_SFTP_OPEN, 0, 0);
return 0;
}
// seems that PHP calls stream_read in 8k chunks
call_user_func($this->notification, STREAM_NOTIFY_PROGRESS, STREAM_NOTIFY_SEVERITY_INFO, '', 0, strlen($chunk), $this->size);
}
// seems that PHP calls stream_read in 8k chunks
call_user_func($this->notification, STREAM_NOTIFY_PROGRESS, STREAM_NOTIFY_SEVERITY_INFO, '', 0, strlen($result), $this->size);

$this->buffer .= $chunk;
}

$result = substr($this->buffer, 0, $count);
$this->buffer = substr($this->buffer, $count);
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicky but... you could do $result = \phpseclib3\Common\Functions\Strings::shift($this->buffer, $count) instead. Also, I'd do $this->buffer.= $chunk instead of $this->buffer .= $chunk.


if (empty($result)) { // ie. false or empty string
$this->eof = true;
return false;
Expand Down