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: reused DAO sockets don't set the keyspace. fix #170 #191

Closed
wants to merge 2 commits into from

Conversation

thibaultcha
Copy link
Member

DAO sockets now take advantage of the connection pool and if they are
coming from it (meaning they've been used before), they don't set their
keyspace anymore.

DAO sockets now take advantage of the connection pool and if they are
coming from it (meaning they've been used before), they don't set their
keyspace anymore.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 65.87% when pulling 0792948 on fix/reused-sockets into f065809 on master.

return nil, DaoError(err, error_types.DATABASE)
end

if times == 0 or not times then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how lua works, but wouldn't you want to check for not times first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah that's a legacy check, only the 0 is actually needed, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops my bad, it was a still needed check. About the order, it is correct. Most of the time while in use, and in need of performance (when Kong will be running), get_reused_times() will return a number (0 possibly). And thus our first condition need to be that.

If we are in a phase of NGINX that doesn't support the cosocket API (init_by_lua) or maybe not even in the NGINX context (the DAO is also used by the CLI or by the unit tests), get_reused_times() can also error and return nil. That is a less common case and not in need of performance.

Hence why the check. My bad for thinking I could remove it, because I reorganized the code before pushing but overlooked at it. I closed this PR by mistake, and had to reopen it in #197 tho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants