Skip to content

Commit 2265a43

Browse files
dlechdlech
committed
Fix race condition when browsing for connected devices
Event handlers were connected after browsing started, so device added events could be missed, causing devices to not be listed in the quickpick menu. This is fixed by adding new start and stop methods to the browser classes on each backend. This way the object can be created, event listeners attached, then the browser can be started.
1 parent f367ae2 commit 2265a43

6 files changed

Lines changed: 121 additions & 76 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how
1010
- Stop button does not kill all child processes
1111
- Activate extension on command palette command
1212
- Fix multiple network interfaces not updated on Windows when scanning for devices
13+
- Fix race condition when browsing for connected devices
1314
### Added
1415
- ev3dev remote debugger is now a default debugger for Python files
1516

‎src/device.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,7 @@ export class Device extends vscode.Disposable {
555555
const selectedItem = await new Promise<ServiceItem>(async (resolve, reject) => {
556556
// start browsing for devices
557557
const dnssdClient = await Device.getDnssdClient();
558-
const browser = await dnssdClient.browse({
558+
const browser = await dnssdClient.createBrowser({
559559
ipv: 'IPv6',
560560
service: 'sftp-ssh'
561561
});
@@ -597,6 +597,8 @@ export class Device extends vscode.Disposable {
597597
reject(err);
598598
});
599599

600+
await browser.start();
601+
600602
while (!done) {
601603
cancelSource = new vscode.CancellationTokenSource();
602604
// using this promise in the quick-pick will cause a progress

‎src/dnssd.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ import * as bonjour from './dnssd/bonjour';
88
*/
99
export interface Client {
1010
/**
11-
* Start browsing
11+
* Create a new browser object.
1212
*/
13-
browse(options: BrowseOptions): Promise<Browser>;
13+
createBrowser(options: BrowseOptions): Promise<Browser>;
1414

1515
/**
1616
* Frees resources used by client and destroys any associated browsers, etc.
@@ -40,9 +40,19 @@ export interface BrowseOptions {
4040
ipv?: 'IPv4' | 'IPv6';
4141
}
4242

43+
/** Object for monitoring network service discovery events. */
4344
export interface Browser {
44-
on(event: 'added' | 'removed', listener: (service: Service) => void): this;
45+
/** Registers callback for service added events. */
46+
on(event: 'added', listener: (service: Service) => void): this;
47+
/** Registers callback for service removed events. */
48+
on(event: 'removed', listener: (service: Service) => void): this;
49+
/** Registers callback for error events. */
4550
on(event: 'error', listener: (err: Error) => void): this;
51+
/** Starts browsing. */
52+
start(): Promise<void>;
53+
/** Stops browsing. */
54+
stop(): Promise<void>;
55+
/** Frees all resources used by browser. */
4656
destroy(): void;
4757
}
4858

‎src/dnssd/avahi.ts‎

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ class AvahiClient implements dnssd.Client {
7373
constructor(readonly server: Server) {
7474
}
7575

76-
public browse(options: dnssd.BrowseOptions): Promise<dnssd.Browser> {
76+
public createBrowser(options: dnssd.BrowseOptions): Promise<dnssd.Browser> {
7777
return new Promise((resolve, reject) => {
7878
const browser = new AvahiBrowser(this, options);
7979
browser.once('ready', () => {
@@ -118,15 +118,14 @@ class AvahiClient implements dnssd.Client {
118118
class AvahiBrowser extends events.EventEmitter implements dnssd.Browser {
119119
private browser: ServiceBrowser | undefined;
120120
private readonly services: AvahiService[] = new Array<AvahiService>();
121+
private readonly bus: dbus.MessageBus;
121122

122-
constructor(client: AvahiClient, private options: dnssd.BrowseOptions) {
123+
constructor(private readonly client: AvahiClient, private options: dnssd.BrowseOptions) {
123124
super();
124-
const proto = this.options.ipv === 'IPv6' ? PROTO_INET6 : PROTO_INET;
125-
const type = `_${this.options.service}._${this.options.transport || 'tcp'}`;
126125
// @ts-ignore
127-
const bus: dbus.MessageBus = client.server.$object.bus;
126+
this.bus = client.server.$object.bus;
128127
// @ts-ignore
129-
bus.on('message', (msg: dbus.Message) => {
128+
this.bus.on('message', (msg: dbus.Message) => {
130129
if (msg.type !== dbus.MessageType.SIGNAL) {
131130
return;
132131
}
@@ -168,28 +167,25 @@ class AvahiBrowser extends events.EventEmitter implements dnssd.Browser {
168167
signature: 's',
169168
body: [`type='signal',sender='org.freedesktop.Avahi',interface='org.freedesktop.Avahi.ServiceBrowser'`]
170169
});
171-
bus.call(addMatchMessage).then(async () => {
172-
const objPath = await client.server.ServiceBrowserNew(IF_UNSPEC, proto, type, '', 0);
173-
const proxy = await bus.getProxyObject('org.freedesktop.Avahi', objPath);
174-
this.browser = proxy.getInterface<ServiceBrowser>('org.freedesktop.Avahi.ServiceBrowser');
175-
this.emit('ready');
176-
// HACK: the current browser model is racy - starts browsing before
177-
// event listeners are added. So we replay the events later so they
178-
// aren't missed.
179-
setTimeout(() => {
180-
for (const s of this.services) {
181-
this.emit('added', s);
182-
}
183-
}, 500);
184-
}).catch((err) => this.emit('error', err));
170+
this.bus.call(addMatchMessage).then(() => this.emit('ready')).catch((err) => this.emit('error', err));
171+
}
172+
173+
public async start(): Promise<void> {
174+
const proto = this.options.ipv === 'IPv6' ? PROTO_INET6 : PROTO_INET;
175+
const type = `_${this.options.service}._${this.options.transport || 'tcp'}`;
176+
const objPath = await this.client.server.ServiceBrowserNew(IF_UNSPEC, proto, type, '', 0);
177+
const proxy = await this.bus.getProxyObject('org.freedesktop.Avahi', objPath);
178+
this.browser = proxy.getInterface<ServiceBrowser>('org.freedesktop.Avahi.ServiceBrowser');
179+
}
180+
181+
public async stop(): Promise<void> {
182+
await this.browser?.Free();
183+
this.browser = undefined;
185184
}
186185

187186
destroy(): void {
188187
this.removeAllListeners();
189-
if (this.browser) {
190-
this.browser.Free().catch(err => console.error(err));
191-
this.browser = undefined;
192-
}
188+
this.stop();
193189
}
194190

195191
}

‎src/dnssd/bonjour.ts‎

Lines changed: 60 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ class BonjourClient extends events.EventEmitter implements dnssd.Client {
2020
}
2121
}
2222

23-
public browse(opts: dnssd.BrowseOptions): Promise<dnssd.Browser> {
23+
public createBrowser(opts: dnssd.BrowseOptions): Promise<dnssd.Browser> {
2424
const browser = new BonjourBrowser(this, opts);
2525
return Promise.resolve(browser);
2626
}
@@ -129,25 +129,50 @@ class BonjourClient extends events.EventEmitter implements dnssd.Client {
129129
}
130130
}
131131

132+
/** Per-client browser object. */
133+
type ClientBrowser = {
134+
/** Bonjour client associated with specific network interface and address. */
135+
bClient: bonjour.Bonjour,
136+
/** Bonjour browser for the Bonjour client. */
137+
browser: bonjour.Browser,
138+
/** Services discovered by the browser. */
139+
services: BonjourService[],
140+
/** Update timer - undefined if not started. */
141+
updateInterval?: NodeJS.Timer,
142+
};
143+
132144
class BonjourBrowser extends events.EventEmitter implements dnssd.Browser {
133-
private readonly browsers = new Array<{
134-
bClient: bonjour.Bonjour,
135-
browser: bonjour.Browser,
136-
services: BonjourService[],
137-
updateInterval: NodeJS.Timer,
138-
}>();
145+
private started = false;
146+
private readonly browsers = new Array<ClientBrowser>();
139147

140148
constructor(private readonly client: BonjourClient, private readonly opts: dnssd.BrowseOptions) {
141149
super();
142-
client.on('clientAdded', c => this.addBrowser(c));
143-
client.on('clientRemoved', c => this.removeBrowser(c));
150+
this.addBrowser = this.addBrowser.bind(this);
151+
this.removeBrowser = this.removeBrowser.bind(this);
152+
client.on('clientAdded', this.addBrowser);
153+
client.on('clientRemoved', this.removeBrowser);
144154
client.forEachClient(c => this.addBrowser(c));
145155
}
146156

147-
public destroy(): void {
157+
public async start(): Promise<void> {
148158
for (const b of this.browsers) {
149-
b.browser.stop();
159+
this.startClientBrowser(b);
150160
}
161+
this.started = true;
162+
}
163+
164+
public async stop(): Promise<void> {
165+
for (const b of this.browsers) {
166+
this.stopClientBrowser(b);
167+
}
168+
this.started = false;
169+
}
170+
171+
public destroy(): void {
172+
this.removeAllListeners();
173+
this.client.off('clientAdded', this.addBrowser);
174+
this.client.off('clientRemoved', this.removeBrowser);
175+
this.stop();
151176
}
152177

153178
private addBrowser(bClient: bonjour.Bonjour) {
@@ -176,24 +201,39 @@ class BonjourBrowser extends events.EventEmitter implements dnssd.Browser {
176201
const [service] = services.splice(index, 1);
177202
this.emit('removed', service, false);
178203
});
179-
this.browsers.push({
180-
bClient: bClient, browser: browser, services: services, updateInterval: setInterval(() => {
181-
// poll again every 1 second
182-
browser.update();
183-
}, 1000)
184-
});
185-
browser.start();
204+
const clientBrowser = { bClient: bClient, browser: browser, services: services };
205+
this.browsers.push(clientBrowser);
206+
207+
// If a new client is added after we have already started browsing, we need
208+
// to start that browser as well.
209+
if (this.started) {
210+
this.startClientBrowser(clientBrowser);
211+
}
186212
}
187213

188214
private removeBrowser(bClient: bonjour.Bonjour): void {
189215
const i = this.browsers.findIndex(v => v.bClient === bClient);
190216
const [removed] = this.browsers.splice(i, 1);
191-
clearInterval(removed.updateInterval);
192-
removed.browser.stop();
217+
this.stopClientBrowser(removed);
193218
for (const s of removed.services) {
194219
this.emit('removed', s);
195220
}
196221
}
222+
223+
private startClientBrowser(clientBrowser: ClientBrowser): void {
224+
clientBrowser.browser.start();
225+
clientBrowser.updateInterval = setInterval(() => {
226+
// poll again every 1 second
227+
clientBrowser.browser.update();
228+
}, 1000);
229+
}
230+
231+
private stopClientBrowser(clientBrowser: ClientBrowser): void {
232+
if (clientBrowser.updateInterval) {
233+
clearInterval(clientBrowser.updateInterval);
234+
clientBrowser.browser.stop();
235+
}
236+
}
197237
}
198238

199239
class BonjourService implements dnssd.Service {

‎src/dnssd/dnssd.ts‎

Lines changed: 24 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,9 @@ class DnssdClient implements dnssd.Client {
1717
private destroyOps = new Array<() => void>();
1818

1919
// interface method implementation
20-
public browse(options: dnssd.BrowseOptions): Promise<dnssd.Browser> {
21-
return new Promise((resolve, reject) => {
22-
const browser = new DnssdBrowser(this, options);
23-
browser.once('ready', () => {
24-
browser.removeAllListeners('error');
25-
resolve(browser);
26-
});
27-
browser.once('error', err => {
28-
browser.removeAllListeners('ready');
29-
reject(err);
30-
});
31-
});
20+
public createBrowser(options: dnssd.BrowseOptions): Promise<dnssd.Browser> {
21+
const browser = new DnssdBrowser(this, options);
22+
return Promise.resolve(browser);
3223
}
3324

3425
// interface method implementation
@@ -60,19 +51,20 @@ class DnssdClient implements dnssd.Client {
6051
}
6152

6253
class DnssdBrowser extends events.EventEmitter implements dnssd.Browser {
63-
private running = false;
6454
private service: dns.Service | undefined;
6555
private destroyOp: () => void;
6656
readonly services: DnssdService[] = new Array<DnssdService>();
6757

6858
constructor(private dnssd: DnssdClient, private options: dnssd.BrowseOptions) {
6959
super();
7060
this.destroyOp = this.dnssd.pushDestroyOp(() => this.destroy());
61+
}
7162

63+
public async start(): Promise<void> {
7264
const regType = `_${this.options.service}._${this.options.transport || 'tcp'}`;
7365
const domain = ''; // TODO: is this part of options?
7466

75-
dns.Service.browse(0, 0, regType, domain, async (s, f, i, e, n, t, d) => {
67+
this.service = await dns.Service.browse(0, 0, regType, domain, async (s, f, i, e, n, t, d) => {
7668
if (e) {
7769
this.emit('error', new dns.ServiceError(e, 'Error while browsing.'));
7870
return;
@@ -110,25 +102,29 @@ class DnssdBrowser extends events.EventEmitter implements dnssd.Browser {
110102
this.emit('removed', service);
111103
}
112104
}
113-
}).then(async service => {
114-
this.service = service;
115-
this.running = true;
116-
this.emit('ready');
117-
while (this.running) {
118-
await service.processResult();
119-
}
120-
}).catch(err => {
121-
this.emit('error', err);
122105
});
106+
107+
// process received results in the background
108+
(async () => {
109+
while (this.service) {
110+
try {
111+
await this.service.processResult();
112+
} catch (err) {
113+
this.emit('error', err);
114+
}
115+
}
116+
})();
117+
}
118+
119+
public async stop(): Promise<void> {
120+
this.service?.destroy();
121+
this.service = undefined;
123122
}
124123

125124
destroy(): void {
125+
this.removeAllListeners();
126+
this.stop();
126127
this.dnssd.popDestroyOp(this.destroyOp);
127-
this.running = false;
128-
if (this.service) {
129-
this.service.destroy();
130-
this.service = undefined;
131-
}
132128
}
133129
}
134130

0 commit comments

Comments
 (0)