Skip to content

Commit f746440

Browse files
author
epriestley
committed
Limit memory usage of ssh-exec during large pull operations
Summary: Fixes T4241. Ref T4206. See T4241 for a description here. Generally, when we connect a fat pipe (`git-upload-pack`) to a narrow one (`git` over SSH) we currently read limitless data into memory. Instead, throttle reads until writes catch up. This is now possible because of the previous changes in this sequence. Test Plan: - Ran `git clone` and `git push` on the entire Wine repository. - Observed CPU and memory usage. - Memory usage was constant and low, CPU usage was high only during I/O (which is expected, since we have to actually do work, although thre might be room to further reduce this). Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4241, T4206 Differential Revision: https://secure.phabricator.com/D7776
1 parent 537f2ea commit f746440

File tree

1 file changed

+45
-3
lines changed

1 file changed

+45
-3
lines changed

src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,14 +97,48 @@ public function execute() {
9797

9898
$channels = array($command_channel, $io_channel, $error_channel);
9999

100+
// We want to limit the amount of data we'll hold in memory for this
101+
// process. See T4241 for a discussion of this issue in general.
102+
103+
$buffer_size = (1024 * 1024); // 1MB
104+
$io_channel->setReadBufferSize($buffer_size);
105+
$command_channel->setReadBufferSize($buffer_size);
106+
107+
// TODO: This just makes us throw away stderr after the first 1MB, but we
108+
// don't currently have the support infrastructure to buffer it correctly.
109+
// It's difficult to imagine this causing problems in practice, though.
110+
$this->execFuture->getStderrSizeLimit($buffer_size);
111+
100112
while (true) {
101113
PhutilChannel::waitForAny($channels);
102114

103115
$io_channel->update();
104116
$command_channel->update();
105117
$error_channel->update();
106118

107-
$done = !$command_channel->isOpen();
119+
// If any channel is blocked on the other end, wait for it to flush before
120+
// we continue reading. For example, if a user is running `git clone` on
121+
// a 1GB repository, the underlying `git-upload-pack` may
122+
// be able to produce data much more quickly than we can send it over
123+
// the network. If we don't throttle the reads, we may only send a few
124+
// MB over the I/O channel in the time it takes to read the entire 1GB off
125+
// the command channel. That leaves us with 1GB of data in memory.
126+
127+
while ($command_channel->isOpen() &&
128+
$io_channel->isOpenForWriting() &&
129+
($command_channel->getWriteBufferSize() >= $buffer_size ||
130+
$io_channel->getWriteBufferSize() >= $buffer_size ||
131+
$error_channel->getWriteBufferSize() >= $buffer_size)) {
132+
PhutilChannel::waitForActivity(array(), $channels);
133+
$io_channel->update();
134+
$command_channel->update();
135+
$error_channel->update();
136+
}
137+
138+
// If the subprocess has exited and we've read everything from it,
139+
// we're all done.
140+
$done = !$command_channel->isOpenForReading() &&
141+
$command_channel->isReadBufferEmpty();
108142

109143
$in_message = $io_channel->read();
110144
if ($in_message !== null) {
@@ -133,12 +167,20 @@ public function execute() {
133167

134168
// If the client has disconnected, kill the subprocess and bail.
135169
if (!$io_channel->isOpenForWriting()) {
136-
$this->execFuture->resolveKill();
170+
$this->execFuture
171+
->setStdoutSizeLimit(0)
172+
->setStderrSizeLimit(0)
173+
->setReadBufferSize(null)
174+
->resolveKill();
137175
break;
138176
}
139177
}
140178

141-
list($err) = $this->execFuture->resolve();
179+
list($err) = $this->execFuture
180+
->setStdoutSizeLimit(0)
181+
->setStderrSizeLimit(0)
182+
->setReadBufferSize(null)
183+
->resolve();
142184

143185
return $err;
144186
}

0 commit comments

Comments
 (0)