Merge pull request #77 from bugsnag/start-delivery-when-ready

Don't start delivery until the app is ready

packages/delivery-electron/delivery.d.ts

@@ -1,6 +1,6 @@
 import { Delivery } from '@bugsnag/core/client'
 import { Client } from '@bugsnag/core'
 
-declare const delivery: (filestore: any, net: any) => (client: Client) => Delivery
+declare const delivery: (filestore: any, net: any, app: any) => (client: Client) => Delivery
 
 export default delivery

packages/delivery-electron/delivery.js

@@ -4,7 +4,7 @@ const PayloadQueue = require('./queue')
 const PayloadDeliveryLoop = require('./payload-loop')
 const NetworkStatus = require('./network-status')
 
-const delivery = (client, filestore, net) => {
+const delivery = (client, filestore, net, app) => {
   const send = (opts, body, cb) => {
     const req = net.request(opts, response => {
       if (isOk(response)) {
@@ -34,7 +34,7 @@ const delivery = (client, filestore, net) => {
   }
 
   const syncPlugin = client.getPlugin('stateSync')
-  const statusUpdater = new NetworkStatus(syncPlugin, net)
+  const statusUpdater = new NetworkStatus(syncPlugin, net, app)
   const { queues } = initRedelivery(filestore.getPaths(), statusUpdater, client._logger, send)
 
   const hash = payload => {
@@ -61,12 +61,15 @@ const delivery = (client, filestore, net) => {
             'Bugsnag-Sent-At': (new Date()).toISOString()
           }
         }
-        if (event.attemptImmediateDelivery === false) {
+
+        if (event.attemptImmediateDelivery === false || statusUpdater.isConnected === false) {
           enqueue('event', { opts, body })
           return cb(null)
         }
+
         const { errorClass, errorMessage } = event.events[0].errors[0]
         client._logger.info(`Sending event ${errorClass}: ${errorMessage}`)
+
         send(opts, body, err => {
           if (err) return onerror(err, { opts, body }, 'event', cb)
           cb(null)
@@ -93,7 +96,14 @@ const delivery = (client, filestore, net) => {
             'Bugsnag-Sent-At': (new Date()).toISOString()
           }
         }
+
+        if (statusUpdater.isConnected === false) {
+          enqueue('session', { opts, body })
+          return cb(null)
+        }
+
         client._logger.info('Sending session')
+
         send(opts, body, err => {
           if (err) return onerror(err, { opts, body }, 'session', cb)
           cb(null)
@@ -149,4 +159,4 @@ const isRetryable = status => {
 
 const isOk = response => [200, 202].includes(response.statusCode)
 
-module.exports = (filestore, net) => client => delivery(client, filestore, net)
+module.exports = (filestore, net, app) => client => delivery(client, filestore, net, app)

packages/delivery-electron/network-status.d.ts

@@ -1,7 +1,7 @@
 import EventEmitter from 'events'
 
 declare class NetworkStatus {
-  constructor (plugin: { emitter: EventEmitter }, net?: any = {})
+  constructor (plugin: { emitter: EventEmitter }, net: any, app: any)
   watch (watcher: (state: boolean) => void)
 
   isConnected: boolean

packages/delivery-electron/network-status.js

@@ -1,6 +1,18 @@
 module.exports = class NetworkStatus {
-  constructor ({ emitter }, net = {}) {
-    this.isConnected = typeof net.online === 'boolean' ? net.online : true
+  constructor ({ emitter }, net, app) {
+    this.isReady = app.isReady()
+
+    // if the app isn't ready, emit an update when it is
+    if (!this.isReady) {
+      app.whenReady().then(() => {
+        this.isReady = true
+        this._update(net.online)
+      })
+    }
+
+    // the net module can't be used if the app is ready, so act as if we're
+    // offline until then
+    this.isConnected = this.isReady ? net.online : false
     this._watchers = []
 
     emitter.on('MetadataUpdate', ({ section, values }) => {
@@ -22,7 +34,11 @@ module.exports = class NetworkStatus {
   }
 
   _update (isConnected) {
-    if (typeof isConnected !== 'boolean' || isConnected === this.isConnected) return
+    // ignore the update if the app is not ready or this is a duplicate
+    if (typeof isConnected !== 'boolean' || isConnected === this.isConnected || this.isReady === false) {
+      return
+    }
+
     this.isConnected = isConnected
     this._watchers.forEach(w => w(this.isConnected))
   }

packages/delivery-electron/test/delivery.test-main.ts

@@ -1,5 +1,5 @@
 import { createServer, IncomingHttpHeaders, STATUS_CODES } from 'http'
-import { net } from 'electron'
+import { app, net } from 'electron'
 import { AddressInfo } from 'net'
 import delivery from '../'
 import { EventDeliveryPayload } from '@bugsnag/core/client'
@@ -25,6 +25,8 @@ const makeClient = (config: any = {}, logger: any = noopLogger) => {
   } as unknown as Client
 }
 
+const nextTick = async () => await new Promise(resolve => process.nextTick(resolve))
+
 jest.mock('../queue')
 jest.mock('../payload-loop')
 
@@ -81,7 +83,8 @@ describe('delivery: electron', () => {
 
   it('sends events successfully', done => {
     const { requests, server } = mockServer()
-    server.listen((err: any) => {
+    // @ts-expect-error the types for 'listen' don't include this overload
+    server.listen(0, 'localhost', (err: any) => {
       expect(err).toBeUndefined()
 
       const payload = {
@@ -92,7 +95,7 @@ describe('delivery: electron', () => {
         endpoints: { notify: `http://localhost:${(server.address() as AddressInfo).port}/notify/` },
         redactedKeys: []
       }
-      delivery(filestore, net)(makeClient(config)).sendEvent(payload, (err: any) => {
+      delivery(filestore, net, app)(makeClient(config)).sendEvent(payload, (err: any) => {
         expect(err).toBe(null)
         expect(requests.length).toBe(1)
         expect(requests[0].method).toBe('POST')
@@ -112,7 +115,8 @@ describe('delivery: electron', () => {
 
   it('sends sessions successfully', done => {
     const { requests, server } = mockServer(202)
-    server.listen((err: any) => {
+    // @ts-expect-error the types for 'listen' don't include this overload
+    server.listen(0, 'localhost', (err: any) => {
       expect(err).toBeUndefined()
 
       const payload = {
@@ -123,7 +127,7 @@ describe('delivery: electron', () => {
         endpoints: { notify: 'blah', sessions: `http://localhost:${(server.address() as AddressInfo).port}/sessions/` },
         redactedKeys: []
       }
-      delivery(filestore, net)(makeClient(config)).sendSession(payload, (err: any) => {
+      delivery(filestore, net, app)(makeClient(config)).sendSession(payload, (err: any) => {
         expect(err).toBe(null)
         expect(requests.length).toBe(1)
         expect(requests[0].method).toBe('POST')
@@ -152,7 +156,7 @@ describe('delivery: electron', () => {
     }
     let didLog = false
     const logger = { error: () => { didLog = true }, info: () => {} }
-    delivery(filestore, net)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
+    delivery(filestore, net, app)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
       expect(didLog).toBe(true)
       expect(err).toBeTruthy()
       expect(enqueueSpy).toHaveBeenCalledWith(
@@ -179,7 +183,8 @@ describe('delivery: electron', () => {
 
   it('handles errors gracefully (400)', done => {
     const { requests, server } = mockServer(400)
-    server.listen((err: any) => {
+    // @ts-expect-error the types for 'listen' don't include this overload
+    server.listen(0, 'localhost', (err: any) => {
       expect(err).toBeUndefined()
 
       const payload = {
@@ -192,7 +197,7 @@ describe('delivery: electron', () => {
       }
       let didLog = false
       const logger = { error: () => { didLog = true }, info: () => {} }
-      delivery(filestore, net)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
+      delivery(filestore, net, app)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
         expect(didLog).toBe(true)
         expect(enqueueSpy).not.toHaveBeenCalled()
         expect(err).toBeTruthy()
@@ -214,7 +219,7 @@ describe('delivery: electron', () => {
     }
     let didLog = false
     const logger = { error: () => { didLog = true }, info: () => {} }
-    delivery(filestore, net)(makeClient(config, logger)).sendSession(payload, (err: any) => {
+    delivery(filestore, net, app)(makeClient(config, logger)).sendSession(payload, (err: any) => {
       expect(didLog).toBe(true)
       expect(err).toBeTruthy()
       expect(enqueueSpy).toHaveBeenCalledWith(
@@ -243,7 +248,8 @@ describe('delivery: electron', () => {
       req.connection.destroy()
     })
 
-    server.listen((err: any) => {
+    // @ts-expect-error the types for 'listen' don't include this overload
+    server.listen(0, 'localhost', (err: any) => {
       expect(err).toBeFalsy()
       const payload = {
         events: [{ errors: [{ errorClass: 'Error', errorMessage: 'foo is not a function' }] }]
@@ -255,7 +261,7 @@ describe('delivery: electron', () => {
       }
       let didLog = false
       const logger = { error: () => { didLog = true }, info: () => {} }
-      delivery(filestore, net)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
+      delivery(filestore, net, app)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
         expect(didLog).toBe(true)
         expect(err).toBeTruthy()
         expect(enqueueSpy).toHaveBeenCalled()
@@ -272,7 +278,8 @@ describe('delivery: electron', () => {
       res.end('NOT OK')
     })
 
-    server.listen((err: any) => {
+    // @ts-expect-error the types for 'listen' don't include this overload
+    server.listen(0, 'localhost', (err: any) => {
       expect(err).toBeFalsy()
       const payload = {
         events: [{ errors: [{ errorClass: 'Error', errorMessage: 'foo is not a function' }] }]
@@ -284,7 +291,7 @@ describe('delivery: electron', () => {
       }
       let didLog = false
       const logger = { error: () => { didLog = true }, info: () => {} }
-      delivery(filestore, net)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
+      delivery(filestore, net, app)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
         expect(didLog).toBe(true)
         expect(err).toBeTruthy()
         expect(enqueueSpy).toHaveBeenCalled()
@@ -307,7 +314,7 @@ describe('delivery: electron', () => {
       endpoints: { notify: 'http://localhost:9999/events/' },
       redactedKeys: []
     }
-    delivery(filestore, net)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
+    delivery(filestore, net, app)(makeClient(config, logger)).sendEvent(payload, (err: any) => {
       expect(didLog).toBe(true)
       expect(err).toBeTruthy()
       expect(enqueueSpy).toHaveBeenCalledWith(
@@ -340,7 +347,7 @@ describe('delivery: electron', () => {
       endpoints: { sessions: 'http://localhost:9999/sessions/' },
       redactedKeys: []
     }
-    delivery(filestore, net)(makeClient(config, logger)).sendSession(payload, (err: any) => {
+    delivery(filestore, net, app)(makeClient(config, logger)).sendSession(payload, (err: any) => {
       expect(didLog).toBe(true)
       expect(err).toBeTruthy()
       expect(enqueueSpy).toHaveBeenCalledWith(
@@ -373,7 +380,7 @@ describe('delivery: electron', () => {
       endpoints: { notify: 'https://some-address.com' },
       redactedKeys: []
     }
-    delivery(filestore, net)(makeClient(config)).sendEvent(payload, (err: any) => {
+    delivery(filestore, net, app)(makeClient(config)).sendEvent(payload, (err: any) => {
       expect(err).not.toBeTruthy()
       expect(enqueueSpy).toHaveBeenCalled()
       done()
@@ -387,7 +394,7 @@ describe('delivery: electron', () => {
     } as any))
 
     const net = { online: true }
-    delivery(filestore, net)(makeClient())
+    delivery(filestore, net, app)(makeClient())
   })
 
   it('does not start the redelivery loop if there is no connection', done => {
@@ -397,36 +404,53 @@ describe('delivery: electron', () => {
     } as any))
 
     const net = { online: false }
-    delivery(filestore, net)(makeClient())
+    delivery(filestore, net, app)(makeClient())
   })
 
-  it('starts the redelivery loop when a connection becomes available', done => {
-    PayloadDeliveryLoopMock.mockImplementation(() => ({
-      start: done,
-      stop: () => {}
-    } as any))
+  it('starts the redelivery loop when a connection becomes available', async () => {
+    const start = jest.fn()
+    const stop = jest.fn()
+
+    PayloadDeliveryLoopMock.mockImplementation(() => ({ start, stop } as any))
 
     const net = { online: false }
     const client = makeClient()
     const emitter = new EventEmitter()
     client.getPlugin = (_name: string) => { return { emitter } }
-    delivery(filestore, net)(client)
+
+    delivery(filestore, net, app)(client)
 
     emitter.emit('MetadataUpdate', { section: 'device', values: { online: true } }, null)
+
+    await nextTick()
+
+    expect(start).toHaveBeenCalled()
+    expect(stop).not.toHaveBeenCalled()
   })
 
-  it('stops the redelivery loop when a connection is lost', done => {
-    PayloadDeliveryLoopMock.mockImplementation(() => ({
-      start: () => {},
-      stop: done
-    } as any))
+  it('stops the redelivery loop when a connection is lost', async () => {
+    const start = jest.fn()
+    const stop = jest.fn()
+
+    PayloadDeliveryLoopMock.mockImplementation(() => ({ start, stop } as any))
 
     const net = { online: true }
     const client = makeClient()
     const emitter = new EventEmitter()
     client.getPlugin = (_name: string) => { return { emitter } }
-    delivery(filestore, net)(client)
+
+    delivery(filestore, net, app)(client)
+
+    await nextTick()
+
+    expect(start).toHaveBeenCalled()
+    expect(stop).not.toHaveBeenCalled()
 
     emitter.emit('MetadataUpdate', { section: 'device', values: { online: false } }, null)
+
+    await nextTick()
+
+    expect(start).toHaveBeenCalled()
+    expect(stop).toHaveBeenCalled()
   })
 })