Skip to content

Commit 4cf4ef1

Browse files
akshita31akshita31
authored
Merge pull request #2245 from akshita31/use_buffer
Use a buffer instead of a temporary file for download and install
2 parents 48fb2ba + f03195d commit 4cf4ef1

File tree

8 files changed

+45
-113
lines changed

8 files changed

+45
-113
lines changed

‎src/omnisharp/OmnisharpDownloader.ts‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,12 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import * as fs from 'fs';
76
import { GetPackagesFromVersion } from './OmnisharpPackageCreator';
87
import { PlatformInformation } from '../platform';
98
import { PackageInstallation, LogPlatformInfo, InstallationSuccess, InstallationFailure, LatestBuildDownloadStart } from './loggingEvents';
109
import { EventStream } from '../EventStream';
1110
import { NetworkSettingsProvider } from '../NetworkSettings';
1211
import { DownloadAndInstallPackages } from '../packageManager/PackageManager';
13-
import { CreateTmpFile, TmpAsset } from '../CreateTmpAsset';
1412
import { DownloadFile } from '../packageManager/FileDownloader';
1513
import { ResolveFilePaths } from '../packageManager/PackageFilePathResolver';
1614

@@ -43,23 +41,16 @@ export class OmnisharpDownloader {
4341
}
4442

4543
public async GetLatestVersion(serverUrl: string, latestVersionFileServerPath: string): Promise<string> {
46-
let description = "Latest Omnisharp Version Information";
44+
let description = "Latest OmniSharp Version Information";
4745
let url = `${serverUrl}/${latestVersionFileServerPath}`;
48-
let tmpFile: TmpAsset;
4946
try {
5047
this.eventStream.post(new LatestBuildDownloadStart());
51-
tmpFile = await CreateTmpFile();
52-
await DownloadFile(tmpFile.fd, description, this.eventStream, this.networkSettingsProvider, url);
53-
return fs.readFileSync(tmpFile.name, 'utf8');
48+
let versionBuffer = await DownloadFile(description, this.eventStream, this.networkSettingsProvider, url);
49+
return versionBuffer.toString('utf8');
5450
}
5551
catch (error) {
5652
this.eventStream.post(new InstallationFailure('getLatestVersionInfoFile', error));
5753
throw error;
5854
}
59-
finally {
60-
if (tmpFile) {
61-
tmpFile.dispose();
62-
}
63-
}
6455
}
6556
}

‎src/packageManager/FileDownloader.ts‎

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,20 @@
55

66
import * as https from 'https';
77
import * as util from '../common';
8-
import * as fs from 'fs';
98
import { EventStream } from "../EventStream";
109
import { DownloadSuccess, DownloadStart, DownloadFallBack, DownloadFailure, DownloadProgress, DownloadSizeObtained } from "../omnisharp/loggingEvents";
1110
import { NestedError } from "../NestedError";
1211
import { parse as parseUrl } from 'url';
1312
import { getProxyAgent } from './proxy';
1413
import { NetworkSettingsProvider } from '../NetworkSettings';
1514

16-
export async function DownloadFile(destinationFileDescriptor: number, description: string, eventStream: EventStream, networkSettingsProvider: NetworkSettingsProvider, url: string, fallbackUrl?: string){
15+
export async function DownloadFile(description: string, eventStream: EventStream, networkSettingsProvider: NetworkSettingsProvider, url: string, fallbackUrl?: string) {
1716
eventStream.post(new DownloadStart(description));
18-
17+
1918
try {
20-
await downloadFile(destinationFileDescriptor, description, url, eventStream, networkSettingsProvider);
19+
let buffer = await downloadFile(description, url, eventStream, networkSettingsProvider);
2120
eventStream.post(new DownloadSuccess(` Done!`));
21+
return buffer;
2222
}
2323
catch (primaryUrlError) {
2424
// If the package has a fallback Url, and downloading from the primary Url failed, try again from
@@ -27,8 +27,9 @@ export async function DownloadFile(destinationFileDescriptor: number, descriptio
2727
if (fallbackUrl) {
2828
eventStream.post(new DownloadFallBack(fallbackUrl));
2929
try {
30-
await downloadFile(destinationFileDescriptor, description, fallbackUrl, eventStream, networkSettingsProvider);
30+
let buffer = await downloadFile(description, fallbackUrl, eventStream, networkSettingsProvider);
3131
eventStream.post(new DownloadSuccess(' Done!'));
32+
return buffer;
3233
}
3334
catch (fallbackUrlError) {
3435
throw primaryUrlError;
@@ -40,7 +41,7 @@ export async function DownloadFile(destinationFileDescriptor: number, descriptio
4041
}
4142
}
4243

43-
async function downloadFile(fd: number, description: string, urlString: string, eventStream: EventStream, networkSettingsProvider: NetworkSettingsProvider): Promise<void> {
44+
async function downloadFile(description: string, urlString: string, eventStream: EventStream, networkSettingsProvider: NetworkSettingsProvider): Promise<Buffer> {
4445
const url = parseUrl(urlString);
4546
const networkSettings = networkSettingsProvider();
4647
const proxy = networkSettings.proxy;
@@ -53,15 +54,13 @@ async function downloadFile(fd: number, description: string, urlString: string,
5354
rejectUnauthorized: util.isBoolean(strictSSL) ? strictSSL : true
5455
};
5556

56-
return new Promise<void>((resolve, reject) => {
57-
if (fd == 0) {
58-
reject(new NestedError("Temporary package file unavailable"));
59-
}
57+
let buffers: any[] = [];
6058

59+
return new Promise<Buffer>((resolve, reject) => {
6160
let request = https.request(options, response => {
6261
if (response.statusCode === 301 || response.statusCode === 302) {
6362
// Redirect - download from new location
64-
return resolve(downloadFile(fd, description, response.headers.location, eventStream, networkSettingsProvider));
63+
return resolve(downloadFile(description, response.headers.location, eventStream, networkSettingsProvider));
6564
}
6665

6766
else if (response.statusCode != 200) {
@@ -74,12 +73,12 @@ async function downloadFile(fd: number, description: string, urlString: string,
7473
let packageSize = parseInt(response.headers['content-length'], 10);
7574
let downloadedBytes = 0;
7675
let downloadPercentage = 0;
77-
let tmpFile = fs.createWriteStream(null, { fd });
7876

7977
eventStream.post(new DownloadSizeObtained(packageSize));
8078

8179
response.on('data', data => {
8280
downloadedBytes += data.length;
81+
buffers.push(data);
8382

8483
// Update status bar item with percentage
8584
let newPercentage = Math.ceil(100 * (downloadedBytes / packageSize));
@@ -90,15 +89,12 @@ async function downloadFile(fd: number, description: string, urlString: string,
9089
});
9190

9291
response.on('end', () => {
93-
resolve();
92+
resolve(Buffer.concat(buffers));
9493
});
9594

9695
response.on('error', err => {
97-
reject(new NestedError(`Reponse error: ${err.message || 'NONE'}`, err));
96+
reject(new NestedError(`Failed to download from ${urlString}. Error Message: ${err.message} || 'NONE'}`, err));
9897
});
99-
100-
// Begin piping data from the response to the package file
101-
response.pipe(tmpFile, { end: false });
10298
});
10399

104100
request.on('error', err => {

‎src/packageManager/PackageManager.ts‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,19 +12,16 @@ import { InstallZip } from './ZipInstaller';
1212
import { EventStream } from '../EventStream';
1313
import { NetworkSettingsProvider } from "../NetworkSettings";
1414
import { filterPackages } from "./PackageFilterer";
15-
import { CreateTmpFile, TmpAsset } from "../CreateTmpAsset";
1615

1716
// Package manager needs a list of packages to be filtered based on platformInfo then download and install them
1817
// Note that the packages that this component will install needs absolute paths for the installPath, intsallTestPath and the binaries
1918
export async function DownloadAndInstallPackages(packages: Package[], provider: NetworkSettingsProvider, platformInfo: PlatformInformation, eventStream: EventStream) {
2019
let filteredPackages = await filterPackages(packages, platformInfo);
2120
if (filteredPackages) {
22-
let tmpFile: TmpAsset;
2321
for (let pkg of filteredPackages) {
2422
try {
25-
tmpFile = await CreateTmpFile();
26-
await DownloadFile(tmpFile.fd, pkg.description, eventStream, provider, pkg.url, pkg.fallbackUrl);
27-
await InstallZip(tmpFile.fd, pkg.description, pkg.installPath, pkg.binaries, eventStream);
23+
let buffer = await DownloadFile(pkg.description, eventStream, provider, pkg.url, pkg.fallbackUrl);
24+
await InstallZip(buffer, pkg.description, pkg.installPath, pkg.binaries, eventStream);
2825
}
2926
catch (error) {
3027
if (error instanceof NestedError) {
@@ -34,12 +31,6 @@ export async function DownloadAndInstallPackages(packages: Package[], provider:
3431
throw error;
3532
}
3633
}
37-
finally {
38-
//clean the temporary file
39-
if (tmpFile) {
40-
tmpFile.dispose();
41-
}
42-
}
4334
}
4435
}
4536
}

‎src/packageManager/ZipInstaller.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,11 @@ import { EventStream } from "../EventStream";
1111
import { InstallationStart } from "../omnisharp/loggingEvents";
1212
import { NestedError } from '../NestedError';
1313

14-
export async function InstallZip(sourceFileDescriptor: number, description: string, destinationInstallPath: string, binaries: string[], eventStream: EventStream): Promise<void> {
14+
export async function InstallZip(buffer: Buffer, description: string, destinationInstallPath: string, binaries: string[], eventStream: EventStream): Promise<void> {
1515
eventStream.post(new InstallationStart(description));
1616

1717
return new Promise<void>((resolve, reject) => {
18-
if (sourceFileDescriptor == 0) {
19-
return reject(new NestedError('Downloaded file unavailable'));
20-
}
21-
22-
yauzl.fromFd(sourceFileDescriptor, { lazyEntries: true }, (err, zipFile) => {
18+
yauzl.fromBuffer(buffer, { lazyEntries: true }, (err, zipFile) => {
2319
if (err) {
2420
return reject(new NestedError('Immediate zip file error', err));
2521
}

‎test/unitTests/Packages/FileDownloader.test.ts‎

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,9 @@
55

66
import * as fs from 'async-file';
77
import * as chai from 'chai';
8-
import * as util from '../../../src/common';
98
import { EventStream } from '../../../src/EventStream';
109
import { DownloadFile } from '../../../src/packageManager/FileDownloader';
1110
import NetworkSettings from '../../../src/NetworkSettings';
12-
import { TmpAsset, CreateTmpFile } from '../../../src/CreateTmpAsset';
1311
import { BaseEvent, DownloadStart, DownloadSizeObtained, DownloadProgress, DownloadSuccess, DownloadFallBack, DownloadFailure } from '../../../src/omnisharp/loggingEvents';
1412
import { getRequestHandler } from '../testAssets/MockHttpServerRequestHandler';
1513

@@ -48,7 +46,6 @@ suite("FileDownloader", () => {
4846

4947
let server: any;
5048
let httpsServerUrl: string;
51-
let tmpFile: TmpAsset;
5249

5350
suiteSetup(async () => {
5451
let port = await getPort();
@@ -66,8 +63,6 @@ suite("FileDownloader", () => {
6663

6764
setup(async () => {
6865
await new Promise(resolve => server.start(resolve));
69-
tmpFile = await CreateTmpFile();
70-
util.setExtensionPath(tmpFile.name);
7166
eventBus = [];
7267
server.on(getRequestHandler('GET', correctUrlPath, 200, { "content-type": "text/plain" }, "Test content"));
7368
server.on(getRequestHandler('GET', errorUrlPath, 404));
@@ -92,15 +87,13 @@ suite("FileDownloader", () => {
9287
].forEach((elem) => {
9388
suite(elem.description, () => {
9489
test('File is downloaded', async () => {
95-
await DownloadFile(tmpFile.fd, fileDescription, eventStream, networkSettingsProvider, getURL(elem.urlPath), getURL(elem.fallBackUrlPath));
96-
const stats = await fs.stat(tmpFile.name);
97-
expect(stats.size).to.not.equal(0);
98-
let text = await fs.readFile(tmpFile.name, 'utf8');
90+
let buffer = await DownloadFile(fileDescription, eventStream, networkSettingsProvider, getURL(elem.urlPath), getURL(elem.fallBackUrlPath));
91+
let text = buffer.toString('utf8');
9992
expect(text).to.be.equal("Test content");
10093
});
10194

10295
test('Events are created in the correct order', async () => {
103-
await DownloadFile(tmpFile.fd, fileDescription, eventStream, networkSettingsProvider, getURL(elem.urlPath), getURL(elem.fallBackUrlPath));
96+
await DownloadFile(fileDescription, eventStream, networkSettingsProvider, getURL(elem.urlPath), getURL(elem.fallBackUrlPath));
10497
expect(eventBus).to.be.deep.equal(elem.getEventSequence());
10598
});
10699
});
@@ -109,17 +102,15 @@ suite("FileDownloader", () => {
109102

110103
suite('If the response status Code is 301, redirect occurs and the download succeeds', () => {
111104
test('File is downloaded from the redirect url', async () => {
112-
await DownloadFile(tmpFile.fd, fileDescription, eventStream, networkSettingsProvider, getURL(redirectUrlPath));
113-
const stats = await fs.stat(tmpFile.name);
114-
expect(stats.size).to.not.equal(0);
115-
let text = await fs.readFile(tmpFile.name, "utf8");
105+
let buffer = await DownloadFile(fileDescription, eventStream, networkSettingsProvider, getURL(redirectUrlPath));
106+
let text = buffer.toString('utf8');
116107
expect(text).to.be.equal("Test content");
117108
});
118109
});
119110

120111
suite('If the response status code is not 301, 302 or 200 then the download fails', () => {
121112
test('Error is thrown', async () => {
122-
expect(DownloadFile(tmpFile.fd, fileDescription, eventStream, networkSettingsProvider, getURL(errorUrlPath))).be.rejected;
113+
expect(DownloadFile(fileDescription, eventStream, networkSettingsProvider, getURL(errorUrlPath))).be.rejected;
123114
});
124115

125116
test('Download Start and Download Failure events are created', async () => {
@@ -128,24 +119,16 @@ suite("FileDownloader", () => {
128119
new DownloadFailure("failed (error code '404')")
129120
];
130121
try {
131-
await DownloadFile(tmpFile.fd, fileDescription, eventStream, networkSettingsProvider, getURL(errorUrlPath));
122+
await DownloadFile(fileDescription, eventStream, networkSettingsProvider, getURL(errorUrlPath));
132123
}
133124
catch (error) {
134125
expect(eventBus).to.be.deep.equal(eventsSequence);
135126
}
136127
});
137128
});
138129

139-
test('Error is thrown on invalid input file', async () => {
140-
//fd=0 means there is no file
141-
expect(DownloadFile(0, fileDescription, eventStream, networkSettingsProvider, getURL(errorUrlPath))).to.be.rejected;
142-
});
143-
144130
teardown(async () => {
145131
await new Promise((resolve, reject) => server.stop(resolve));
146-
if (tmpFile) {
147-
tmpFile.dispose();
148-
}
149132
});
150133

151134
function getURL(urlPath: string) {

‎test/unitTests/Packages/PackageManager.test.ts‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,9 @@ const ServerMock = require("mock-http-server");
2323
const getPort = require('get-port');
2424

2525
suite("Package Manager", () => {
26-
let tmpSourceDir: TmpAsset;
2726
let tmpInstallDir: TmpAsset;
2827
let server: any;
2928
let downloadUrl: string;
30-
let testDirPath: string;
3129
let allFiles: Array<{ content: string, path: string }>;
3230
let installationPath: string;
3331
let eventBus: Array<BaseEvent>;
@@ -55,7 +53,6 @@ suite("Package Manager", () => {
5553

5654
setup(async () => {
5755
eventBus = [];
58-
tmpSourceDir = await CreateTmpDir(true);
5956
tmpInstallDir = await CreateTmpDir(true);
6057
installationPath = tmpInstallDir.name;
6158
packages = <Package[]>[
@@ -67,13 +64,12 @@ suite("Package Manager", () => {
6764
architectures: [windowsPlatformInfo.architecture]
6865
}];
6966
allFiles = [...Files, ...Binaries];
70-
testDirPath = tmpSourceDir.name + "/test.zip";
71-
await createTestZipAsync(testDirPath, allFiles);
67+
let buffer = await createTestZipAsync(allFiles);
7268
await new Promise(resolve => server.start(resolve)); //start the server
7369
server.on(getRequestHandler('GET', '/package', 200, {
7470
"content-type": "application/zip",
75-
"content-length": (await fs.stat(testDirPath)).size
76-
}, await fs.readFile(testDirPath)));
71+
"content-length": buffer.length
72+
}, buffer));
7773
});
7874

7975
test("Downloads the package and installs at the specified path", async () => {
@@ -105,9 +101,6 @@ suite("Package Manager", () => {
105101
});
106102

107103
teardown(async () => {
108-
if (tmpSourceDir) {
109-
tmpSourceDir.dispose();
110-
}
111104
if (tmpInstallDir) {
112105
tmpInstallDir.dispose();
113106
}

0 commit comments

Comments
 (0)