HPCC-17017 Add port info to socket tracename #10196

mckellyln · 2017-07-05T02:13:06Z

Signed-off-by: Mark Kelly mark.kelly@lexisnexisrisk.com

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Testing:

Full QA suite passed.

hpcc-jirabot · 2017-07-05T02:13:20Z

https://track.hpccsystems.com/browse/HPCC-17017
Jira updated

mckellyln · 2017-07-05T02:17:26Z

system/jlib/jsocket.cpp

+            strcpy(lname, "(NULL)");
+            lport = 1;
+            e->Release();
+        }


Not related to original enhancement but discovered this and took the opportunity to add try/catch around name().

what about other places that call name() , e.g. logConnectionInfo

Should name() always catch and behave like this?

logConnectionInfo may be ok as its always after a successful connect and if there is an error then throwing may make sense. I will wrap it in try / catch and ignore error if you think that is better.
But in general case - should name() throw ? I am not sure.

richardkchapman · 2017-07-05T08:40:39Z

@jakesmith Please review. Any performance concerns?

jakesmith · 2017-07-05T11:22:03Z

system/jlib/jsocket.cpp


    free(tracename);
-    tracename = strdup(peer);
+    tracename = strdup(peer.str());


Not new, but this would be better as:

tracename = peer.detach();

jakesmith · 2017-07-05T11:22:04Z

system/jlib/jsocket.cpp

    peer.append(name?name:"(NULL)");
+    peer.append(":").append(hostport);
+    if (sock != INVALID_SOCKET)
+        peer.append(" (").append(sock).append(")");


Doesn't feel like this should be done here (in setTraceName), but rather by the caller (Can common up elsewhere if same pattern).
e.g., CSocket::CSocket(const SocketEndpoint &ep, ... ,with name where trace name is "T>"
will have target port already and can use it.

lport is local port, hostport is remote port, good to see both and sock fd in trace.

jakesmith · 2017-07-05T11:24:18Z

system/jlib/jsocket.cpp

+        catch (IException *e)
+        {
+            strcpy(lname, "(NULL)");
+            lport = 1;


sorry, should be -1

jakesmith · 2017-07-05T11:25:54Z

system/jlib/jsocket.cpp

+            strcpy(lname, "(NULL)");
+            lport = 1;
+            e->Release();
+        }


what about other places that call name() , e.g. logConnectionInfo

Should name() always catch and behave like this?

jakesmith · 2017-07-05T11:27:19Z

system/jlib/jsocket.cpp

@@ -2662,7 +2687,7 @@ CSocket::CSocket(const SocketEndpoint &ep,SOCKETMODE smode,const char *name)
        SocketEndpoint self;
        self.setLocalHost(0);
        self.getUrlStr(hostname);
-        setTraceName("S>", hostname);
+        setTraceName("S>", hostname.str());


fine, but not strictly necessary, const char * cast operator will call str() automatically.

mckellyln · 2017-07-05T13:34:42Z

system/jlib/jsocket.cpp


    free(tracename);
-    tracename = strdup(peer);
+    tracename = peer.detach();


used detach()

mckellyln · 2017-07-05T13:34:59Z

system/jlib/jsocket.cpp

+        strcpy(lname, "(NULL)");
+        lport = -1;
+        e->Release();
+    }


Added try / catch here also

mckellyln · 2017-07-05T17:33:37Z

@jakesmith code updated as per review comments and discussion.

jakesmith · 2017-07-06T14:37:30Z

system/jlib/jsocket.cpp

@@ -893,6 +894,8 @@ int CSocket::post_connect ()
        nagling = true;
        set_nagle(false);
        state = ss_open;
+        char lname[256];
+        localPort = name(lname, sizeof(lname), false);


a slight waste getting lname when not needed.
CSocket::name() should fill in a StringBuffer really too, which it does internally, only to copy to the fixed sized passed in buffer.
You could change it to 2 methods:

void CSocket::getEndpoint(SocketEndpoint &ep); unsigned CSocket::getName(StringBuffer &name);

and use getEndpoint() here.

Same is true of CSocket::peer_name()

The oktothrow=false, is it anymore expect to throw an exception here than anywhere else?
The previously (prior to this commit) it would throw because called too early.

jakesmith · 2017-07-06T14:44:16Z

system/security/securesocket/securesocket.cpp

@@ -234,9 +234,9 @@ class CSecureSocket : implements ISecureSocket, public CInterface
    }

    // Get name of accepted socket and returns port
-    virtual int name(char *name,size32_t namemax)
+    virtual int name(char *name, size32_t namemax, bool oktothrow=true)


minor: could use override

jakesmith · 2017-07-06T14:51:29Z

system/jlib/jsocket.cpp

    char peer[256];
-    peer_name(peer,sizeof(peer));
+    hostport = peer_name(peer,sizeof(peer));
+    hostname = strdup(peer);


can remove:

hostname = NULL; hostport = 0;

early on in constructor

jakesmith · 2017-07-06T14:53:29Z

system/jlib/jsocket.cpp

+    hostport = peer_name(peer,sizeof(peer));
+    hostname = strdup(peer);
+    char lname[256];
+    localPort = name(lname, sizeof(lname), false);


It may not be an efficient concern, but as per prev. comment (in post_connect) it would look cleaner if method didn't get lname that it didn't need.

Unless it would be useful to store? I notice e.g. logConnectionInfo used to use, but you removed?

jakesmith · 2017-07-06T14:56:26Z

system/jlib/jsocket.cpp

-    char rname[256];
-    int rport = peer_name(rname, sizeof(rname));
-    PROGLOG("SOCKTRACE: connect(%u) - time:%u ms fd:%d l:%s:%d r:%s:%d", timeoutms, conn_mstime, sock, lname, lport, rname, rport);
+    PROGLOG("SOCKTRACE: connect(%u) - time:%u ms fd:%d l:%d r:%s:%d", timeoutms, conn_mstime, sock, localPort, hostname, hostport);


was logging lname not useful?

jakesmith · 2017-07-06T14:59:25Z

@mckellyln - a few comments.

Am not sure about the CSocket::name change (oktothrow).
see comment where oktothrow=false.
i.e. is it really more expected to throw an exception in that context than others?
If not, then why ignore in that 1 case not others.
Probably should throw there too (?) Socket is open by then now, so should succeed.
If should succeed (as with other contexts) then don't need oktothrow.

mckellyln · 2017-07-07T15:32:01Z

@jakesmith updated as per comments for re-review.

jakesmith · 2017-07-07T16:52:45Z

system/jlib/jsocket.hpp

@@ -331,6 +331,9 @@ class jlib_decl ISocket : extends IInterface
    // Get peer ip of socket - in UDP returns return addr
    virtual IpAddress &getPeerAddress(IpAddress &addr)=0;

+    // Get local endpoint of socket
+    virtual SocketEndpoint &getEndpoint(SocketEndpoint &ep)=0;


trivial: could be const method, but other existing methods (e.g. getPeerAddress) should be too.

jakesmith · 2017-07-07T16:52:53Z

system/security/securesocket/securesocket.cpp

@@ -257,6 +257,12 @@ class CSecureSocket : implements ISecureSocket, public CInterface
        return m_socket->getPeerAddress(addr);
    }

+    // Get local endpoint of socket
+    virtual SocketEndpoint &getEndpoint(SocketEndpoint &ep)


could be const + override.

jakesmith · 2017-07-07T16:53:29Z

A couple of trivial comment, but looks good to merge now.
@richardkchapman

mckellyln · 2017-07-07T17:26:48Z

@jakesmith added const + override to new functions. If you are ok with last commit, I will squash all commits.

jakesmith · 2017-07-07T17:38:21Z

@mckellyln - Yes looks good, go ahead and squash.

Signed-off-by: Mark Kelly <mark.kelly@lexisnexisrisk.com>

mckellyln · 2017-07-07T17:48:15Z

@jakesmith, @richardkchapman squashed and ready for merge.

HPCCSmoketest · 2017-07-08T02:24:04Z

Automated Smoketest: ✅
Sha: 25e388e
Build: success
Build: success
ECL Watch: Rebuilding Site

errors	warnings	build time
0	65	92.98 seconds

Install hpccsystems-platform-community_6.4.0-rc5.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed
unittest	94	94
wutoolTest(Dali)	19	19
wutoolTest(Cassandra)	19	19

Regression test result:

phase	total	pass
setup (hthor)	11	11
setup (thor)	11	11
setup (roxie)	11	11
test (hthor)	733	733
test (thor)	627	627
test (roxie)	760	760

HPCC Stop: OK
HPCC Uninstall: OK

mckellyln force-pushed the hpcc-17017 branch from 7835599 to ff72341 Compare July 5, 2017 02:15

mckellyln commented Jul 5, 2017

View reviewed changes

mckellyln force-pushed the hpcc-17017 branch from ff72341 to e9dc7a8 Compare July 5, 2017 02:19

jakesmith requested changes Jul 5, 2017

View reviewed changes

mckellyln commented Jul 5, 2017

View reviewed changes

system/jlib/jsocket.cpp

free(tracename);

tracename = strdup(peer);

tracename = peer.detach();

Copy link

Contributor Author

mckellyln Jul 5, 2017

Choose a reason for hiding this comment

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

used detach()

mckellyln commented Jul 5, 2017

View reviewed changes

system/jlib/jsocket.cpp Outdated

strcpy(lname, "(NULL)");

lport = -1;

e->Release();

}

Copy link

Contributor Author

mckellyln Jul 5, 2017

Choose a reason for hiding this comment

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

Added try / catch here also

jakesmith reviewed Jul 6, 2017

View reviewed changes

mckellyln force-pushed the hpcc-17017 branch 3 times, most recently from da27589 to bbd297a Compare July 7, 2017 15:24

jakesmith reviewed Jul 7, 2017

View reviewed changes

HPCC-17017 Add port info to socket tracename

25e388e

Signed-off-by: Mark Kelly <mark.kelly@lexisnexisrisk.com>

mckellyln force-pushed the hpcc-17017 branch from bffe09a to 25e388e Compare July 7, 2017 17:44

richardkchapman merged commit 5f116f8 into hpcc-systems:candidate-6.4.0 Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-17017 Add port info to socket tracename #10196

HPCC-17017 Add port info to socket tracename #10196

mckellyln commented Jul 5, 2017

hpcc-jirabot commented Jul 5, 2017

mckellyln Jul 5, 2017

jakesmith Jul 5, 2017

mckellyln Jul 5, 2017

richardkchapman commented Jul 5, 2017

jakesmith Jul 5, 2017

jakesmith Jul 5, 2017

mckellyln Jul 5, 2017

jakesmith Jul 5, 2017

mckellyln Jul 5, 2017

jakesmith Jul 5, 2017

jakesmith Jul 5, 2017

mckellyln Jul 5, 2017

mckellyln Jul 5, 2017

mckellyln commented Jul 5, 2017

jakesmith Jul 6, 2017

jakesmith Jul 6, 2017

jakesmith Jul 6, 2017

jakesmith Jul 6, 2017

jakesmith Jul 6, 2017

jakesmith Jul 6, 2017

jakesmith commented Jul 6, 2017

mckellyln commented Jul 7, 2017

jakesmith Jul 7, 2017

jakesmith Jul 7, 2017

jakesmith commented Jul 7, 2017

mckellyln commented Jul 7, 2017

jakesmith commented Jul 7, 2017

mckellyln commented Jul 7, 2017

HPCCSmoketest commented Jul 8, 2017

HPCC-17017 Add port info to socket tracename #10196

HPCC-17017 Add port info to socket tracename #10196

Conversation

mckellyln commented Jul 5, 2017

Type of change:

Checklist:

Testing:

hpcc-jirabot commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckellyln commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Jul 6, 2017

mckellyln commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Jul 7, 2017

mckellyln commented Jul 7, 2017

jakesmith commented Jul 7, 2017

mckellyln commented Jul 7, 2017

HPCCSmoketest commented Jul 8, 2017