Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-5064): consolidate connection cleanup logic and ensure socket is always closed #3572

Merged
merged 14 commits into from
Feb 17, 2023

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Feb 15, 2023

Description

What is changing?

This PR primarily does three things:

  • ensures that the message stream is always destroyed and ensure that the socket is always destroyed
  • consolidate the connection clean up logic into a single function that is called from each point where the connection is cleaned out
  • improves the unit tests to properly assert on the state of the connection after _cleanup has been called. Additionally, the unit tests now in for synchronous versus asynchronous behavior for the various error and clean up scenarios.

Example unit test output:

  new Connection()
    ✔ should support fire-and-forget messages
    ✔ should destroy streams which time out (55ms)
    ✔ should throw a network error with kBeforeHandshake set to false on timeout after handshake (56ms)
    ✔ should throw a network error with kBeforeHandshake set to true on timeout before handshake (53ms)
    #onMessage
      when the connection is a monitoring connection
        when multiple hellos exist on the stream
          ✔ calls the callback with the last hello document
        when requestId/responseTo do not match
          ✔ calls the operation description callback with the document
        when requestId/reponseTo match
          ✔ calls the operation description callback with the document
        when no operation description is in the queue
          ✔ does not error
        when more than one operation description is in the queue
          ✔ calls all operation description callbacks with an error
    when the socket times out
      delayed timeout for lambda behavior
        ✔ should delay timeout errors by one tick
        ✔ should clear timeout errors if more data is available
      when the timeout expires and no more data has come in
        ✔ destroys the message stream
        ✔ ends the socket
        ✔ destroys the socket after ending it
        ✔ passes the error along to any callbacks in the operation description queue (asynchronously)
        ✔ emits a Connection.CLOSE event (asynchronously)
        ✔ removes all listeners on the message stream
        ✔ removes all listeners on the socket
    when the message stream errors
      ✔ destroys the message stream synchronously
      ✔ ends the socket
      ✔ destroys the socket after ending it (asynchronously)
      ✔ passes the error along to any callbacks in the operation description queue (asynchronously)
      ✔ emits a Connection.CLOSE event (asynchronously)
      ✔ removes all listeners on the message stream
      ✔ removes all listeners on the socket
    when the underlying socket closes
      ✔ destroys the message stream synchronously
      ✔ ends the socket
      ✔ destroys the socket after ending it (asynchronously)
      ✔ calls any callbacks in the queue with a MongoNetworkError (asynchronously)
      ✔ emits a Connection.CLOSE event (asynchronously)
      ✔ removes all listeners on the message stream
      ✔ removes all listeners on the socket
    .hasSessionSupport
      when logicalSessionTimeoutMinutes is present
        ✔ returns true
      when logicalSessionTimeoutMinutes is not present
        when in load balancing mode
          ✔ returns true
        when not in load balancing mode
          ✔ returns false
    destroy()
      when a callback is provided
        when the connection was already destroyed
          ✔ does not attempt to cleanup the socket again
          ✔ calls the callback (asynchronously)
        when the connection was not destroyed
          ✔ cleans up the connection
          ✔ calls the callback (asynchronously)
      when options.force == true
        ✔ destroys the tcp socket (synchronously)
        ✔ does not call stream.end
        ✔ destroys the messageStream (synchronously)
        ✔ when destroy({ force: true }) is called multiple times, it calls stream.destroy exactly once
      when options.force == false
        ✔ destroys the tcp socket (asynchronously)
        ✔ ends the tcp socket (synchronously)
        ✔ destroys the messageStream (synchronously)
        ✔ calls stream.end exactly once when destroy is called multiple times
Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

src/cmap/connection.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
@baileympearson baileympearson marked this pull request as ready for review February 15, 2023 18:47
@baileympearson baileympearson changed the title make DestroyOptions required on connection.destroy fix(NODE-5064): consolidate connection cleanup logic Feb 15, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 15, 2023
@nbbeeken nbbeeken self-requested a review February 15, 2023 18:52
@baileympearson baileympearson changed the title fix(NODE-5064): consolidate connection cleanup logic fix(NODE-5064): consolidate connection cleanup logic and ensure socket is always closed Feb 15, 2023
addaleax
addaleax previously approved these changes Feb 15, 2023
test/unit/cmap/connection.test.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
test/unit/cmap/connection.test.ts Outdated Show resolved Hide resolved
test/unit/cmap/connection.test.ts Outdated Show resolved Hide resolved
test/unit/cmap/connection.test.ts Outdated Show resolved Hide resolved
test/unit/cmap/connection.test.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Feb 16, 2023
@nbbeeken nbbeeken merged commit e544d88 into main Feb 17, 2023
@nbbeeken nbbeeken deleted the NODE-5064 branch February 17, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants