Fix Service advertisement race conditions #919
Conversation
- Implement operation queue: All advertise/unadvertise operations are now serialized through a promise queue to prevent race conditions - Allow safe re-advertisement: advertise() and advertiseAsync() automatically unadvertise before re-advertising when a service is already advertised - Make operations async: Both methods now return promises that resolve when the operation completes - Prevent double operations: Added internal state tracking to prevent duplicate unadvertise operations - Fix cleanup order: Event callbacks are removed before sending unadvertise messages to stop processing new requests immediately
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes race conditions in Service advertisement operations by implementing a serialized operation queue and automatic re-advertisement handling. The changes ensure that advertise and unadvertise operations can be called safely in any order without errors.
- Implements a promise-based operation queue to serialize all advertise/unadvertise operations
- Allows automatic re-advertisement by unadvertising before re-advertising when a service is already advertised
- Converts operations to return promises and prevents duplicate unadvertise operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/core/Service.js | Implements operation queue, async operation handling, and automatic re-advertisement logic |
| test/service.test.js | Adds comprehensive tests for re-advertisement, multiple unadvertise calls, and operation serialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/core/Service.js
Outdated
| }); | ||
| this.isAdvertised = true; | ||
| }).catch(err => { | ||
| console.error(`Error advertising service ${this.name}:`, err); |
There was a problem hiding this comment.
The error message uses console.error which may not be appropriate for a library. Consider using the EventEmitter's error event or allowing the error to propagate to the caller instead of logging it.
| console.error(`Error advertising service ${this.name}:`, err); | |
| this.emit('error', err); |
Copilot uses AI. Check for mistakes.
src/core/Service.js
Outdated
| this._operationQueue = this._operationQueue.then(async () => { | ||
| await this._doUnadvertise(); | ||
| }).catch(err => { | ||
| console.error(`Error unadvertising service ${this.name}:`, err); |
There was a problem hiding this comment.
The error message uses console.error which may not be appropriate for a library. Consider using the EventEmitter's error event or allowing the error to propagate to the caller instead of logging it.
| console.error(`Error unadvertising service ${this.name}:`, err); | |
| this.emit('error', err); |
Copilot uses AI. Check for mistakes.
src/core/Service.js
Outdated
| }); | ||
| this.isAdvertised = true; | ||
| }).catch(err => { | ||
| console.error(`Error advertising async service ${this.name}:`, err); |
There was a problem hiding this comment.
The error message uses console.error which may not be appropriate for a library. Consider using the EventEmitter's error event or allowing the error to propagate to the caller instead of logging it.
| console.error(`Error advertising async service ${this.name}:`, err); | |
| this.emit('error', err); |
Copilot uses AI. Check for mistakes.
Public API Changes
Service.advertise()now returns aPromise that resolves when the
advertisement is complete
Service.advertiseAsync()now returns aPromise that resolves when the
advertisement is complete
Service.unadvertise()now returns aPromise that resolves when the
unadvertisement is complete
'error' events instead of being logged to
console
Description
operations are now serialized through a promise queue
to prevent race conditions
advertiseAsync() automatically unadvertise before
re-advertising when a service is already advertised
promises that resolve when the operation completes
tracking to prevent duplicate unadvertise operations
sending unadvertise messages to stop processing new
requests immediately