Skip to content

Add Tequilapi client timeout - #460

Merged
Waldz merged 5 commits into
mysteriumnetwork:masterfrom
tcharding:tequilapi-timeout
Oct 22, 2018
Merged

Add Tequilapi client timeout#460
Waldz merged 5 commits into
mysteriumnetwork:masterfrom
tcharding:tequilapi-timeout

Conversation

@tcharding

@tcharding tcharding commented Oct 18, 2018

Copy link
Copy Markdown
Contributor

As described in issue #386 current tequilapi client does not use a timeout. In current go version (1.11) stdlib http.Client does implement timeout functionality (also available in 1.9 and 1.10).

From /usr/lib/go/src/net/http/client.go (golang version 1.11.1):

// A Client is an HTTP client. Its zero value (DefaultClient) is a
// usable client that uses DefaultTransport.
...
type Client struct {
...
// Timeout specifies a time limit for requests made by this
// Client. The timeout includes connection time, any
// redirects, and reading the response body. The timer remains
// running after Get, Head, Post, or Do return and will
// interrupt reading of the Response.Body.
//
// A Timeout of zero means no timeout.
//
// The Client cancels requests to the underlying Transport
// as if the Request's Context ended.
//
// For compatibility, the Client will also use the deprecated
// CancelRequest method on Transport if found. New
// RoundTripper implementations should use the Request's Context
// for cancelation instead of implementing CancelRequest.
Timeout time.Duration

We can use this to add a timeout to the tequilapi client.

First do a couple of clean up patches in tequilapi/client, with this PR applied tequilapi/client/*.go lint cleanly. Use capital letters for acronyms as is standard Golang convention. Then add timeout of 120 seconds to the tequilapi client.

Tested by setting timeout to 1 nanosecond and running client

build/myst/myst cli                                                                                                                           ✱ (git:tequilapi-timeout)
2018-10-18T05:02:55.358443336 [Info] Starting Mysterium Node (source.dev-build)
2018-10-18T05:02:55.361336074 [Info] [Openvpn check] OpenVPN 2.4.4 x86_64-pc-linux-gnu [SSL (OpenSSL)] [LZO] [LZ4] [EPOLL] [PKCS11] [MH/PKTINFO] [AEAD] built on Sep  5 2018
2018-10-18T05:02:55.361379991 [Info] [Openvpn check] library versions: OpenSSL 1.1.0g  2 Nov 2017, LZO 2.08
2018-10-18T05:02:55.36139322 [Info] [Openvpn check] Originally developed by James Yonan
2018-10-18T05:02:55.361395925 [Info] [Openvpn check] Copyright (C) 2002-2017 OpenVPN Technologies, Inc. <sales@openvpn.net>
2018-10-18T05:02:55.361401316 [Info] [Openvpn check] Compile time defines: enable_async_push=no enable_comp_stub=no enable_crypto=yes enable_crypto_ofb_cfb=yes enable_debug=yes enable_def_auth=yes enable_dependency_tracking=no enable_dlopen=unknown enable_dlopen_self=unknown enable_dlopen_self_static=unknown enable_fast_install=needless enable_fragment=yes enable_iproute2=yes enable_libtool_lock=yes enable_lz4=yes enable_lzo=yes enable_maintainer_mode=no enable_management=yes enable_multihome=yes enable_pam_dlopen=no enable_pedantic=no enable_pf=yes enable_pkcs11=yes enable_plugin_auth_pam=yes enable_plugin_down_root=yes enable_plugins=yes enable_port_share=yes enable_selinux=no enable_server=yes enable_shared=yes enable_shared_with_static_runtimes=no enable_silent_rules=no enable_small=no enable_static=yes enable_strict=no enable_strict_options=no enable_systemd=yes enable_werror=no enable_win32_dll=yes enable_x509_alt_username=yes with_aix_soname=aix with_crypto_library=openssl with_gnu_ld=yes with_mem_check=no with_sysroot=no
2018-10-18T05:02:55.361412963 [Info] Using Eth endpoint: https://ropsten.infura.io
2018-10-18T05:02:55.361433519 [Info] Using Eth contract at address: 0xbe5F9CCea12Df756bF4a5Baf4c29A10c3ee7C83B
timeout

Closes: #386

Signed-off-by: Tobin C. Harding me@tobin.cc

Comment thread tequilapi/client/http_client.go Outdated
soffokl
soffokl previously approved these changes Oct 18, 2018

@soffokl soffokl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, but CI build should pass.

@tcharding
tcharding dismissed stale reviews from soffokl via 8ad869d October 18, 2018 06:48
soffokl
soffokl previously approved these changes Oct 18, 2018
Waldz
Waldz previously approved these changes Oct 18, 2018
@tcharding
tcharding dismissed stale reviews from Waldz and soffokl via d16dfad October 19, 2018 03:01
Golang convention is to use capital letters for acronyms.  We should use
the variable identifier baseURL instead of baseUrl.

Replace variable identifier `baseUrl` with `baseURL`.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Golang convention is to use capital letters for acronyms.  We should use
the variable identifier newHTTPClient instead of newHttpClient.

Replace variable identifier `newHttpClient` with `newHTTPClient`.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Currently the tequilapi does not use a timeout.  From github issue #386

	this means that if connection issues arise, the current
	implementation of the tequilapi/client will hang FOREVER

http.Client includes a data member for setting a timeout.

Set timeout to 120 seconds when creating new `http.Client` in tequilapi
client.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
@tcharding

Copy link
Copy Markdown
Contributor Author

I don't know what happened on this branch with all the merges of master. I rebased the original three patches on top of tip of master and used @soffokl's suggestion to fix my code fail (120 * time.Seconds). This means you lost the commit attribution though @soffokl :(

@soffokl soffokl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't worry about applying my suggestion via Github. )

@Waldz Waldz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Waldz
Waldz merged commit 9e01341 into mysteriumnetwork:master Oct 22, 2018
@tcharding
tcharding deleted the tequilapi-timeout branch October 22, 2018 22:43
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.

4 participants