xrootd/client: add bind request, implement support for multiple sockets #288
Conversation
xrootd/client/session.go
Outdated
| pathID xrdproto.PathID | ||
| } | ||
|
|
||
| // pendingRequest is the request that having been sent to the server. |
There was a problem hiding this comment.
s/pendingRequest is the request that having been sent to the server./pendingRequest is a request that has been sent to the remote server./
xrootd/client/session.go
Outdated
| requests map[xrdproto.StreamID][]byte | ||
| requests map[xrdproto.StreamID]pendingRequest | ||
|
|
||
| childsMu sync.RWMutex |
There was a problem hiding this comment.
childs isn't english (I know... english is annoying :P), but children is.
what about s/child/sub/, short for sub-session?
then everything reads better:
subsMu sync.RWMutex
subCreateMu sync.Mutex
subs map[xrdproto.PathID]*session
maxSubs int
freeSubs chan xrdproto.PathIDwhat do you think?
There was a problem hiding this comment.
Thanks!
My bad.
You're right, sub looks way better.
xrootd/client/session.go
Outdated
| requests map[xrdproto.StreamID][]byte | ||
| requests map[xrdproto.StreamID]pendingRequest | ||
|
|
||
| childsMu sync.RWMutex |
There was a problem hiding this comment.
also, you should probably say what the 2 mutexes are for.
IIUC, childCreationMu makes sure we serialize the creation of new "sub" sessions, while childsMu protects the access to the map.
you might want to properly describe this in the documentation of these fields.
xrootd/client/session.go
Outdated
| childCreationMu sync.Mutex | ||
| childs map[xrdproto.PathID]*session | ||
|
|
||
| maxChilds int |
There was a problem hiding this comment.
looking at the usage of maxChilds and freeChilds, it seems to me one could just create the chan xrdproto.PathID with a buffer size of maxChilds. (although I haven't worked out the specifics.)
There was a problem hiding this comment.
The idea of maxChilds is to restrict connections count to the specific value. We can afford up to 255 sub-sessions (due to pathID being byte), however, it's not always a good solution and server may restrict max number of connections. It even can be obtained from the server as part of query request IIRC.
I'm not sure about buffer. IIUC, it doesn't restrict max number of values and there is no way of obtaining buffer size (am I wrong?).
There was a problem hiding this comment.
IIRC, one can use cap to get the buffer size of a channel:
https://play.golang.org/p/hl99j1KG__3
still thinking about it, though :)
Actually, current read \ write tests are already using it. Regarding mock tests - I'll look into it but it looks complicated from first sight because session tries to connect to the "remote" server. :-/ Any ideas regarding test failure? It looks like tests for |
|
travis is in a weird state these days. on my laptop, the |
|
hum... the travis build for "master" (so w/o this PR) is consistently successful:
while this PR is consistently failing (also tried a couple of times.) locally, the tests are ok on my laptop. |
|
did your tests passed locally when running with |
|
Yes, both the |
|
It hangs at which means that somehow binding session (either |
Codecov Report
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
+ Coverage 51.31% 51.45% +0.14%
==========================================
Files 213 214 +1
Lines 22015 22118 +103
==========================================
+ Hits 11297 11381 +84
- Misses 9440 9451 +11
- Partials 1278 1286 +8
Continue to review full report at Codecov.
|
xrootd/client/session.go
Outdated
|
|
||
| maxSubs int | ||
| freeSubs chan xrdproto.PathID | ||
| isSub bool // indicates whether this session is sub-session. |
There was a problem hiding this comment.
s/is sub-session/is a sub-session/
xrootd/client/session.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (sess *session) send(ctx context.Context, streamID xrdproto.StreamID, responseChannel mux.DataRecvChan, header []byte, body []byte, pathID xrdproto.PathID) ([]byte, *mux.Redirection, error) { |
There was a problem hiding this comment.
s/header []byte, body []byte/header, body []byte/
xrootd/client/session.go
Outdated
| @@ -299,3 +438,43 @@ func (sess *session) sign(streamID xrdproto.StreamID, requestID uint16, data []b | |||
|
|
|||
| return wBuffer.Bytes(), nil | |||
| } | |||
|
|
|||
| func newChildSession(ctx context.Context, parent *session) (*session, error) { | |||
There was a problem hiding this comment.
s/newChildSession/newSubSession/
| } | ||
| } | ||
|
|
||
| if !sess.isSub { |
There was a problem hiding this comment.
Yep.
For some reason, bind fails with cross-host bind not allowed on CI. I had a guess that it complains about logins from 4 different hosts (running test session) with same login. However, when I tried to make username include current pid nothing changed, so looks like that's not that case.
Due to failing bind, mux was closed and it looked like "request hangs".
There was a problem hiding this comment.
ok.
I am not sure how much of this is "by design" and how much is "that's just the way it is".
perhaps something to clarify with upstream?
|
So, with the assumption that this CORS-like error comes from the way tests are run under Travis, I guess we can leave the tests to be run when people do 'go test' on their machine, and disable it when on Travis. |
First of all, current tests fallback to the sending data over a single socket. I'm going to add As far as I understand, conditional running is obtained via build tags. So, for example, we can set inside Is it better to use opposite way and run that test only if a specified tag was passed (because it can fail for the same reason for someone else and bother people, especially since |
|
I think using a 'travis' tag is fine at the moment. If vgo fails us, we'll have ample time to adjust before it becomes really mainstream (Feb 2019.) Thanks! |
|
thanks! |
No description provided.