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

Bluebird remove return, reflect, fromCallback #3483

Merged
merged 6 commits into from Oct 25, 2019

Conversation

@maximelkin
Copy link
Contributor

maximelkin commented Oct 15, 2019

No description provided.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 15, 2019

It seems to be failing on Node 12 consistently.

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 15, 2019

Hm, strange errors, I will research this

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 15, 2019

So error got passed through try-catch, really strange

Error: This socket has been ended by the other party
    at Socket.writeAfterFIN [as write] (net.js:441:14)
    at Connection.write (/home/travis/build/knex/knex/node_modules/mysql2/lib/connection.js:217:17)
    at Connection.writePacket (/home/travis/build/knex/knex/node_modules/mysql2/lib/connection.js:262:12)
    at Quit.start (/home/travis/build/knex/knex/node_modules/mysql2/lib/commands/quit.js:24:16)
    at Quit.execute (/home/travis/build/knex/knex/node_modules/mysql2/lib/commands/command.js:39:22)
    at Connection.handlePacket (/home/travis/build/knex/knex/node_modules/mysql2/lib/connection.js:408:32)
    at Connection.addCommand (/home/travis/build/knex/knex/node_modules/mysql2/lib/connection.js:430:12)
    at Connection.end (/home/travis/build/knex/knex/node_modules/mysql2/lib/connection.js:796:26)
    at internal/util.js:278:30
    at new Promise (<anonymous>)
    at bound end (internal/util.js:277:12)
    at Client_MySQL2.destroyRawConnection (/home/travis/build/knex/knex/lib/dialects/mysql/index.js:85:20)
@maximelkin maximelkin closed this Oct 15, 2019
@maximelkin maximelkin reopened this Oct 15, 2019
@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 16, 2019

connection.end called after last command fail, which cleared all events, so 'error' event emitted to nowhere and emitted as unhandled error

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 16, 2019

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 16, 2019

Before node12

  1. connection.end called
  2. also error emit in 1) synchroniously
  3. catch
  4. finally (removeAllListeners)

Was

  1. connection.end called (process.nextTick error triggered because closed)
  2. Bluebird.catch
  3. error emitted
  4. Bluebird.finally (removeAllListeners)

Now

  1. connection.end (process.nextTick error triggered)
  2. catch (e) {}
  3. finally {} (removeAllListeners)
  4. error emitted

Bluebird solution worked because of additional microtask from .catch?

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 16, 2019

@kibertoad have you any ideas to resolve this?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 16, 2019

@maximelkin Can we add additional listener to catch the error when it is thrown asynchronously?

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 16, 2019

@kibertoad can this trigger leaks? When to unsubscribe this listener?

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 16, 2019

7069ce5 <- here removeAllListeners added

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 21, 2019

@kibertoad that do you think?

Copy link
Member

elhigu left a comment

Dropping that removeAllListeners call seems wrong.

lib/dialects/mysql/index.js Outdated Show resolved Hide resolved
return await end();
} catch (err) {
connection.__knex__disposed = err;
}

This comment has been minimized.

Copy link
@elhigu

elhigu Oct 22, 2019

Member

Is that finally removeAllListeners really unnecessary? Is there test somewhere, which fails if connection has listeners after disposal?

This comment has been minimized.

Copy link
@maximelkin

maximelkin Oct 22, 2019

Author Contributor

I run knex-stress-test for a some minutes (with process.memoryUsage().heapUsed logging)
memory usage is stable, so no leak introduced

This comment has been minimized.

Copy link
@elhigu

elhigu Oct 23, 2019

Member

Sounds promising. Did you try it with both mysql and mysql2 drivers?

Though if that listener removal is not necessary, shouldn't this function just return end() and leave it to caller to set __knex__disposed like in every other driver 🤔

One more thing... if that Error: This socket has been ended by the other party is now not leaked, I suppose that error is still happening, but it is ignored somewhere (by existence of some event handler maybe). So when it is ignored how does that get recorded to connection.__knex__disposed or does it even matter...

Also I'm pretty confused how is it possible that error is leaking and where that leak is happening exactly (stack trace showed lib/dialects/mysql/index.js:85:20, but then it should have been handled by caller's error handler). And when it is leaked why caller's error handling is not catching it? That was an async function after all.

This is a part of really fundamental functionality of knex and may cause hard to catch failures for huge user base if it is not working properly.I would really like to understand the problem properly before trying to patch it.

This comment has been minimized.

Copy link
@elhigu

elhigu Oct 23, 2019

Member

Can I easily reproduce the original error to investigate it a bit more? Did it happen in some very specific test or every time?

This comment has been minimized.

Copy link
@maximelkin

maximelkin Oct 23, 2019

Author Contributor

Install 12 node
Add finally block with removeAllListeners
run test "#2321 dead connections are not evicted from pool"

lib/dialects/oracle/index.js Outdated Show resolved Hide resolved
lib/dialects/postgres/index.js Outdated Show resolved Hide resolved
lib/dialects/sqlite3/index.js Outdated Show resolved Hide resolved
lib/transaction.js Show resolved Hide resolved
@maximelkin maximelkin force-pushed the maximelkin:bluebird_cleanup branch from d7e428a to 3269fb1 Oct 22, 2019
@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 22, 2019

Dropping that removeAllListeners call seems wrong.

@elhigu why?

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Oct 23, 2019

Dropping that removeAllListeners call seems wrong.

@elhigu why?

Because its addition didn't look like an accident and there was not proper investigation done, if it can be removed or not.

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 24, 2019

As temporal solution we can add delay before removeAllListeners, so it would work like before

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 25, 2019

@maximelkin I believe https://lodash.com/docs/4.17.14#defer does exactly this?

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 25, 2019

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 25, 2019

But i still think what we should review removeAllListeners usage, maybe in separate issue

But with defer, to match all behavior
@maximelkin maximelkin closed this Oct 25, 2019
@maximelkin maximelkin reopened this Oct 25, 2019
@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 25, 2019

@kibertoad kibertoad merged commit 03ecbee into knex:master Oct 25, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maximelkin maximelkin deleted the maximelkin:bluebird_cleanup branch Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.