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

Set timeout for custom dialer. #1035

Merged
merged 2 commits into from
Nov 21, 2019
Merged

Set timeout for custom dialer. #1035

merged 2 commits into from
Nov 21, 2019

Conversation

zjj
Copy link
Contributor

@zjj zjj commented Nov 20, 2019

Description

the dial ctx shall be something todo with the cfg.Timeout

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

connector.go Outdated

dialCtx = dctx
}

Copy link
Member

Choose a reason for hiding this comment

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

Move this block into if ok { block.

Copy link
Contributor Author

@zjj zjj Nov 20, 2019

Choose a reason for hiding this comment

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

updated, feeling something weird, It seems to have to move all them insied

connector.go Show resolved Hide resolved
connector.go Outdated
} else {
nd := net.Dialer{Timeout: mc.cfg.Timeout}
mc.netConn, err = nd.DialContext(ctx, mc.cfg.Net, mc.cfg.Addr)
mc.netConn, err = nd.DialContext(ctx, mc.cfg.Net, mc.cfg.Addr) // for DialContext shall be ctx, not dialCtx
Copy link
Member

Choose a reason for hiding this comment

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

This comment is redundant.

Copy link
Contributor Author

@zjj zjj Nov 20, 2019

Choose a reason for hiding this comment

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

yes, this explains why the dialctx not passed to DialContext, for later puller could get some info.
if you can't accept this redundant comment, I would comit another patch to remove this comment.

remove ?

Copy link
Member

Choose a reason for hiding this comment

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

this explains why the dialctx not passed to DialContext, for later puller could get some info.

Read the comment. It doesn't explain why. And the code tells why cleary.
So this comment is just a noise. Please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@methane
Copy link
Member

methane commented Nov 20, 2019

"Added myself / the copyright holder to the AUTHORS file"

AUTHORS Outdated
@@ -86,6 +86,7 @@ Xiangyu Hu <xiangyu.hu at outlook.com>
Xiaobing Jiang <s7v7nislands at gmail.com>
Xiuming Chen <cc at cxm.cc>
Zhenye Xie <xiezhenye at gmail.com>
Jiajia Zhong <zhong2plus at gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

See line 9:

# Please keep the list sorted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks to your gude.

@methane methane changed the title fix connect dial context #1034 Set timeout for custom dialer. Nov 21, 2019
@methane methane merged commit 15462c1 into go-sql-driver:master Nov 21, 2019
@julienschmidt julienschmidt added this to the v1.5.0 milestone Jan 3, 2020
bitcoinbarron2 added a commit to bitcoinbarron2/mysql that referenced this pull request Feb 18, 2020
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
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