From 3ffcb0c8cfbd87b949fd1e10f8876383f7c4f7f5 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 7 Sep 2017 16:54:34 +0300 Subject: [PATCH] Analytics fixes Move lockfile service to common and lock operation for writing user settings file Add new lockfile service (use lockfile npm package) and lock all operations for reading/writing user-settings.json. These operations may be executed in different processes, so they must be synced between them. Clean obsolete code for lockfile in services that do not longer use it. Ensure failures in eqatec analytics will not prevent tracking in google analytics by try-catching execution in broker service. --- lib/bootstrap.ts | 1 - lib/common | 2 +- lib/declarations.d.ts | 6 --- lib/definitions/lockfile.d.ts | 24 ---------- lib/lockfile.ts | 31 ------------ .../analytics/analytics-broker-process.ts | 2 + lib/services/analytics/analytics-broker.ts | 47 ++++++++++--------- lib/services/analytics/analytics-service.ts | 13 ++++- lib/services/user-settings-service.ts | 9 +++- npm-shrinkwrap.json | 12 +++-- package.json | 3 +- test/npm-installation-manager.ts | 1 - test/npm-support.ts | 2 - test/platform-commands.ts | 1 - test/platform-service.ts | 1 - test/plugins-service.ts | 1 - test/project-service.ts | 1 - 17 files changed, 57 insertions(+), 100 deletions(-) delete mode 100644 lib/definitions/lockfile.d.ts delete mode 100644 lib/lockfile.ts diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts index f4ad743cdd..6d2c0cf6b1 100644 --- a/lib/bootstrap.ts +++ b/lib/bootstrap.ts @@ -71,7 +71,6 @@ $injector.require("itmsTransporterService", "./services/itmstransporter-service" $injector.requirePublic("npm", "./node-package-manager"); $injector.require("npmInstallationManager", "./npm-installation-manager"); -$injector.require("lockfile", "./lockfile"); $injector.require("dynamicHelpProvider", "./dynamic-help-provider"); $injector.require("mobilePlatformsCapabilities", "./mobile-platforms-capabilities"); $injector.require("commandsServiceProvider", "./providers/commands-service-provider"); diff --git a/lib/common b/lib/common index 804e28c137..6522b37bc2 160000 --- a/lib/common +++ b/lib/common @@ -1 +1 @@ -Subproject commit 804e28c137442922609caced942dee3d6512523b +Subproject commit 6522b37bc2beadfccb98f9f1ca9002c7046650b4 diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index d6d40af9f1..c6f18218d7 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -318,12 +318,6 @@ interface IApplicationPackage { time: Date; } -interface ILockFile { - lock(): void; - unlock(): void; - check(): boolean; -} - interface IOpener { open(target: string, appname: string): void; } diff --git a/lib/definitions/lockfile.d.ts b/lib/definitions/lockfile.d.ts deleted file mode 100644 index af62919fee..0000000000 --- a/lib/definitions/lockfile.d.ts +++ /dev/null @@ -1,24 +0,0 @@ -declare module "lockfile" { - export function lock(lockFilename: string, lockParams: ILockParams, callback: (err: Error) => void): void; - export function lockSync(lockFilename: string, lockParams: ILockSyncParams): void; - export function unlock(lockFilename: string, callback: (err: Error) => void): void; - export function unlockSync(lockFilename: string): void; - export function check(lockFilename: string, lockParams: ILockParams, callback: (err: Error, isLocked: boolean) => void): boolean; - export function checkSync(path: string, opts: Options): boolean; - - export interface Options { - wait?: number; - stale?: number; - retries?: number; - retryWait?: number; - } - - interface ILockSyncParams { - retries: number; - stale: number; - } - - interface ILockParams extends ILockSyncParams { - retryWait: number; - } -} diff --git a/lib/lockfile.ts b/lib/lockfile.ts deleted file mode 100644 index d9e0082274..0000000000 --- a/lib/lockfile.ts +++ /dev/null @@ -1,31 +0,0 @@ -import * as lockfile from "lockfile"; -import * as path from "path"; - -export class LockFile implements ILockFile { - private lockFilePath: string; - - constructor(private $options: IOptions) { - this.lockFilePath = path.join(this.$options.profileDir, ".lock"); - } - - private static LOCK_EXPIRY_PERIOD_SEC = 180; - private static LOCK_PARAMS = { - retryWait: 100, - retries: LockFile.LOCK_EXPIRY_PERIOD_SEC * 10, - stale: LockFile.LOCK_EXPIRY_PERIOD_SEC * 1000 - }; - - public lock(): void { - lockfile.lockSync(this.lockFilePath, LockFile.LOCK_PARAMS); - } - - public unlock(): void { - lockfile.unlockSync(this.lockFilePath); - } - - public check(): boolean { - return lockfile.checkSync(this.lockFilePath, LockFile.LOCK_PARAMS); - } -} - -$injector.register("lockfile", LockFile); diff --git a/lib/services/analytics/analytics-broker-process.ts b/lib/services/analytics/analytics-broker-process.ts index 78769ae8ea..02b806aa75 100644 --- a/lib/services/analytics/analytics-broker-process.ts +++ b/lib/services/analytics/analytics-broker-process.ts @@ -1,3 +1,5 @@ +// NOTE: This file is used to track data in a separate process. +// The instances here are not shared with the ones in main CLI process. import * as fs from "fs"; import { AnalyticsBroker } from "./analytics-broker"; diff --git a/lib/services/analytics/analytics-broker.ts b/lib/services/analytics/analytics-broker.ts index 471840cd7b..25d4bf1c3f 100644 --- a/lib/services/analytics/analytics-broker.ts +++ b/lib/services/analytics/analytics-broker.ts @@ -17,29 +17,32 @@ export class AnalyticsBroker implements IAnalyticsBroker { private $injector: IInjector) { } public async sendDataForTracking(trackInfo: ITrackingInformation): Promise { - const eqatecProvider = await this.getEqatecAnalyticsProvider(); - const googleProvider = await this.getGoogleAnalyticsProvider(); - - switch (trackInfo.type) { - case TrackingTypes.Exception: - await eqatecProvider.trackError(trackInfo); - break; - case TrackingTypes.Feature: - await eqatecProvider.trackInformation(trackInfo); - break; - case TrackingTypes.AcceptTrackFeatureUsage: - await eqatecProvider.acceptFeatureUsageTracking(trackInfo); - break; - case TrackingTypes.GoogleAnalyticsData: - await googleProvider.trackHit(trackInfo); - break; - case TrackingTypes.Finish: - await eqatecProvider.finishTracking(); - break; - default: - throw new Error(`Invalid tracking type: ${trackInfo.type}`); + try { + const eqatecProvider = await this.getEqatecAnalyticsProvider(); + const googleProvider = await this.getGoogleAnalyticsProvider(); + + switch (trackInfo.type) { + case TrackingTypes.Exception: + await eqatecProvider.trackError(trackInfo); + break; + case TrackingTypes.Feature: + await eqatecProvider.trackInformation(trackInfo); + break; + case TrackingTypes.AcceptTrackFeatureUsage: + await eqatecProvider.acceptFeatureUsageTracking(trackInfo); + break; + case TrackingTypes.GoogleAnalyticsData: + await googleProvider.trackHit(trackInfo); + break; + case TrackingTypes.Finish: + await eqatecProvider.finishTracking(); + break; + default: + throw new Error(`Invalid tracking type: ${trackInfo.type}`); + } + } catch (err) { + // So, lets ignore the error for now until we find out what to do with it. } - } } diff --git a/lib/services/analytics/analytics-service.ts b/lib/services/analytics/analytics-service.ts index a15f7f7583..a999e1a96d 100644 --- a/lib/services/analytics/analytics-service.ts +++ b/lib/services/analytics/analytics-service.ts @@ -65,10 +65,11 @@ export class AnalyticsService extends AnalyticsServiceBase { public async trackEventActionInGoogleAnalytics(data: IEventActionData): Promise { const device = data.device; const platform = device ? device.deviceInfo.platform : data.platform; + const normalizedPlatform = platform ? this.$mobileHelper.normalizePlatformName(platform) : platform; const isForDevice = device ? !device.isEmulator : data.isForDevice; let label: string = ""; - label = this.addDataToLabel(label, platform); + label = this.addDataToLabel(label, normalizedPlatform); // In some cases (like in case action is Build and platform is Android), we do not know if the deviceType is emulator or device. // Just exclude the device_type in this case. @@ -182,7 +183,15 @@ export class AnalyticsService extends AnalyticsServiceBase { private async sendMessageToBroker(message: ITrackingInformation): Promise { const broker = await this.getAnalyticsBroker(); - return new Promise((resolve, reject) => broker.send(message, resolve)); + return new Promise((resolve, reject) => { + if (broker && broker.connected) { + try { + broker.send(message, resolve); + } catch (err) { + this.$logger.trace("Error while trying to send message to broker:", err); + } + } + }); } } diff --git a/lib/services/user-settings-service.ts b/lib/services/user-settings-service.ts index 07201c799f..a89c8495e1 100644 --- a/lib/services/user-settings-service.ts +++ b/lib/services/user-settings-service.ts @@ -3,9 +3,14 @@ import * as userSettingsServiceBaseLib from "../common/services/user-settings-se class UserSettingsService extends userSettingsServiceBaseLib.UserSettingsServiceBase { constructor($fs: IFileSystem, - $options: IOptions) { + $options: IOptions, + $lockfile: ILockFile) { const userSettingsFilePath = path.join($options.profileDir, "user-settings.json"); - super(userSettingsFilePath, $fs); + super(userSettingsFilePath, $fs, $lockfile); + } + + public async loadUserSettingsFile(): Promise { + await this.loadUserSettingsData(); } } $injector.register("userSettingsService", UserSettingsService); diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 185087f936..86732bf537 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -37,6 +37,12 @@ "@types/node": "6.0.61" } }, + "@types/lockfile": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/@types/lockfile/-/lockfile-1.0.0.tgz", + "integrity": "sha512-pD6JuijPmrfi84qF3/TzGQ7zi0QIX+d7ZdetD6jUA6cp+IsCzAquXZfi5viesew+pfpOTIdAVKuh1SHA7KeKzg==", + "dev": true + }, "@types/node": { "version": "6.0.61", "resolved": "https://registry.npmjs.org/@types/node/-/node-6.0.61.tgz", @@ -3405,9 +3411,9 @@ } }, "lockfile": { - "version": "1.0.1", - "resolved": "https://registry.npmjs.org/lockfile/-/lockfile-1.0.1.tgz", - "integrity": "sha1-nTU+z+P1TRULtX+J1RdGk1o5xPU=" + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/lockfile/-/lockfile-1.0.3.tgz", + "integrity": "sha1-Jjj8OaAzHpysGgS3F5mTHJxQ33k=" }, "lodash": { "version": "4.13.1", diff --git a/package.json b/package.json index 35cc0262d7..ec9320ba40 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,7 @@ "ios-device-lib": "0.4.9", "ios-mobileprovision-finder": "1.0.10", "ios-sim-portable": "3.1.1", - "lockfile": "1.0.1", + "lockfile": "1.0.3", "lodash": "4.13.1", "log4js": "1.0.1", "marked": "0.3.6", @@ -87,6 +87,7 @@ "@types/chai": "4.0.1", "@types/chai-as-promised": "0.0.31", "@types/chokidar": "1.6.0", + "@types/lockfile": "1.0.0", "@types/node": "6.0.61", "@types/qr-image": "3.2.0", "@types/request": "0.0.45", diff --git a/test/npm-installation-manager.ts b/test/npm-installation-manager.ts index f36ff54e90..fa06505ca0 100644 --- a/test/npm-installation-manager.ts +++ b/test/npm-installation-manager.ts @@ -15,7 +15,6 @@ function createTestInjector(): IInjector { testInjector.register("config", ConfigLib.Configuration); testInjector.register("logger", LoggerLib.Logger); - testInjector.register("lockfile", {}); testInjector.register("errors", ErrorsLib.Errors); testInjector.register("options", OptionsLib.Options); testInjector.register("fs", FsLib.FileSystem); diff --git a/test/npm-support.ts b/test/npm-support.ts index 1b9bb23c3c..b4757e924c 100644 --- a/test/npm-support.ts +++ b/test/npm-support.ts @@ -23,7 +23,6 @@ import { ProjectFilesProvider } from "../lib/providers/project-files-provider"; import { MobilePlatformsCapabilities } from "../lib/mobile-platforms-capabilities"; import { DevicePlatformsConstants } from "../lib/common/mobile/device-platforms-constants"; import { XmlValidator } from "../lib/xml-validator"; -import { LockFile } from "../lib/lockfile"; import ProjectChangesLib = require("../lib/services/project-changes-service"); import { Messages } from "../lib/common/messages/messages"; import { NodeModulesDependenciesBuilder } from "../lib/tools/node-modules/node-modules-dependencies-builder"; @@ -48,7 +47,6 @@ function createTestInjector(): IInjector { testInjector.register("platformService", PlatformServiceLib.PlatformService); testInjector.register("logger", stubs.LoggerStub); testInjector.register("npmInstallationManager", {}); - testInjector.register("lockfile", LockFile); testInjector.register("prompter", {}); testInjector.register("sysInfo", {}); testInjector.register("androidProjectService", {}); diff --git a/test/platform-commands.ts b/test/platform-commands.ts index 51dc5acb70..07d11e6430 100644 --- a/test/platform-commands.ts +++ b/test/platform-commands.ts @@ -112,7 +112,6 @@ function createTestInjector() { testInjector.registerCommand("platform|remove", PlatformRemoveCommandLib.RemovePlatformCommand); testInjector.registerCommand("platform|update", PlatformUpdateCommandLib.UpdatePlatformCommand); testInjector.registerCommand("platform|clean", PlatformCleanCommandLib.CleanCommand); - testInjector.register("lockfile", {}); testInjector.register("resources", {}); testInjector.register("commandsServiceProvider", { registerDynamicSubCommands: () => { /* intentionally left blank */ } diff --git a/test/platform-service.ts b/test/platform-service.ts index 8a4faa965a..5c12c376fa 100644 --- a/test/platform-service.ts +++ b/test/platform-service.ts @@ -39,7 +39,6 @@ function createTestInjector() { testInjector.register('projectDataService', stubs.ProjectDataService); testInjector.register('prompter', {}); testInjector.register('sysInfo', {}); - testInjector.register('lockfile', stubs.LockFile); testInjector.register("commandsService", { tryExecuteCommand: () => { /* intentionally left blank */ } }); diff --git a/test/plugins-service.ts b/test/plugins-service.ts index 3fdd2c24f2..6360e2ee0d 100644 --- a/test/plugins-service.ts +++ b/test/plugins-service.ts @@ -69,7 +69,6 @@ function createTestInjector() { registerDynamicSubCommands: () => { /* intentionally empty body */ } }); testInjector.register("hostInfo", HostInfo); - testInjector.register("lockfile", {}); testInjector.register("projectHelper", ProjectHelper); testInjector.register("pluginsService", PluginsService); diff --git a/test/project-service.ts b/test/project-service.ts index d0d3090367..2ed5961274 100644 --- a/test/project-service.ts +++ b/test/project-service.ts @@ -145,7 +145,6 @@ class ProjectIntegrationTest { this.testInjector.register("npmInstallationManager", NpmInstallationManager); this.testInjector.register("npm", NpmLib.NodePackageManager); this.testInjector.register("httpClient", HttpClientLib.HttpClient); - this.testInjector.register("lockfile", stubs.LockFile); this.testInjector.register("options", Options); this.testInjector.register("hostInfo", HostInfo);