fix(changeStream): properly handle changeStream event mid-close #1902
Conversation
If a changeStream gets an event while it is in the middle of closing, a race condition can occur where the event is still processed after the stream has closed. This commit adds handling for this edge case by returning an error to callbacks, rejecting promises, and simply ignoring it in emitter mode. Fixes NODE-1831
lib/change_stream.js
Outdated
| if (changeStream.isClosed()) { | ||
| if (eventEmitter) return; | ||
|
|
||
| const error = new Error('ChangeStream is closed'); |
There was a problem hiding this comment.
I think this should be a MongoError right?
There was a problem hiding this comment.
also nit, but should ChangeStream just be change stream? It makes it look like a product name here 😄
There was a problem hiding this comment.
Was referring to the class ChangeStream here. Figured it was better to specify the constructor object.
Also, I specifically resisted throwing a MongoError b/c right now we use MongoError mostly for server-side errors. We don't really have a concept of a MongoDriverError.
There was a problem hiding this comment.
👍 on point one
For the second point, I still think it makes sense to mark it as a MongoError until such an error class exists. It makes it harder to catch errors generically limited to the driver if we also sometimes throw plain Error s, at very least even if/when a MongoDriverError is introduced it will be a subclass of MongoError
| .then(() => changeStream.next()) | ||
| .then(() => changeStream.next()) | ||
| .then(() => { | ||
| const closeP = Promise.resolve().then(() => changeStream.close()); |
There was a problem hiding this comment.
Interesting, does changeStream.close() not return a Promise? Looks like it should to me. This should at least be: Promise.resolve(changeStream.close())
There was a problem hiding this comment.
This is actually a relic of when I was trying to enqueue changeStream.close as a microTask. I can remove it.
| @@ -1677,4 +1677,133 @@ describe('Change Streams', function() { | |||
| .then(() => finish(), err => finish(err)); | |||
| } | |||
| }); | |||
|
|
|||
| describe('should properly handle a changeStream event being processed mid-close', function() { | |||
There was a problem hiding this comment.
Is it possible to refactor these three tests to use the same setup/teardown code for a client, rather than clutter the tests with it? It also feels like you can use the same method to write data, as you're really only testing different APIs for reads here
…odb#1902) If a changeStream gets an event while it is in the middle of closing, a race condition can occur where the event is still processed after the stream has closed. This commit adds handling for this edge case by returning an error to callbacks, rejecting promises, and simply ignoring it in emitter mode. Fixes NODE-1831
If a changeStream gets an event while it is in the middle of closing,
a race condition can occur where the event is still processed after
the stream has closed. This commit adds handling for this edge
case by returning an error to callbacks, rejecting promises,
and simply ignoring it in emitter mode.
Fixes NODE-1831