change fstat to not reuse request object

eikenb · eikenb · commit 7f106a332761 · 2019-01-29T14:49:22.000-08:00
When fstat is called it now uses the handle to get the opened request,
pulls the filepath from that and creates a new request for the fstat
call. This eliminates the need to change the Method on the request
object and furthers the goal of eliminating post-creation mutation of
the request object to open up the possibility to eliminate or at least
reduce the use of the global request lock.
diff --git a/request-server.go b/request-server.go
@@ -60,33 +60,19 @@ func (rs *RequestServer) nextRequest(r *Request) string {
 	return handle
 }
 
-// Returns Request from openRequests, bool is false if it is missing
-// If the method is different, save/return a new Request w/ that Method.
+// Returns Request from openRequests, bool is false if it is missing.
 //
 // The Requests in openRequests work essentially as open file descriptors that
 // you can do different things with. What you are doing with it are denoted by
-// the first packet of that type (read/write/etc). We create a new Request when
-// it changes to set the request.Method attribute in a thread safe way.
-func (rs *RequestServer) getRequest(handle, method string) (*Request, bool) {
+// the first packet of that type (read/write/etc).
+func (rs *RequestServer) getRequest(handle string) (*Request, bool) {
 	rs.openRequestLock.RLock()
+	defer rs.openRequestLock.RUnlock()
 	r, ok := rs.openRequests[handle]
-	rs.openRequestLock.RUnlock()
-	if !ok || r.Method == method {
-		return r, ok
-	}
-	// if we make it here we need to replace the request
-	rs.openRequestLock.Lock()
-	defer rs.openRequestLock.Unlock()
-	r, ok = rs.openRequests[handle]
-	if !ok || r.Method == method { // re-check needed b/c lock race
-		return r, ok
-	}
-	r = r.copy()
-	r.Method = method
-	rs.openRequests[handle] = r
 	return r, ok
 }
 
+// Close the Request and clear from openRequests map
 func (rs *RequestServer) closeRequest(handle string) error {
 	rs.openRequestLock.Lock()
 	defer rs.openRequestLock.Unlock()
@@ -179,9 +165,18 @@ func (rs *RequestServer) packetWorker(
 			request := requestFromPacket(ctx, pkt)
 			rs.nextRequest(request)
 			rpkt = request.open(rs.Handlers, pkt)
+		case *sshFxpFstatPacket:
+			handle := pkt.getHandle()
+			request, ok := rs.getRequest(handle)
+			if !ok {
+				rpkt = statusFromError(pkt, syscall.EBADF)
+			} else {
+				request = NewRequest("Stat", request.Filepath)
+				rpkt = request.call(rs.Handlers, pkt)
+			}
 		case hasHandle:
 			handle := pkt.getHandle()
-			request, ok := rs.getRequest(handle, requestMethod(pkt))
+			request, ok := rs.getRequest(handle)
 			if !ok {
 				rpkt = statusFromError(pkt, syscall.EBADF)
 			} else {
@@ -201,12 +196,6 @@ func (rs *RequestServer) packetWorker(
 	return nil
 }
 
-// True is responsePacket is an OK status packet
-func statusOk(rpkt responsePacket) bool {
-	p, ok := rpkt.(sshFxpStatusPacket)
-	return ok && p.StatusError.Code == ssh_FX_OK
-}
-
 // clean and return name packet for file
 func cleanPacketPath(pkt *sshFxpRealpathPacket) responsePacket {
 	path := cleanPath(pkt.getPath())
diff --git a/request-server_test.go b/request-server_test.go
@@ -84,7 +84,7 @@ func TestRequestCache(t *testing.T) {
 	fh := p.svr.nextRequest(foo)
 	bh := p.svr.nextRequest(bar)
 	assert.Len(t, p.svr.openRequests, 2)
-	_foo, ok := p.svr.getRequest(fh, "")
+	_foo, ok := p.svr.getRequest(fh)
 	assert.Equal(t, foo.Method, _foo.Method)
 	assert.Equal(t, foo.Filepath, _foo.Filepath)
 	assert.Equal(t, foo.Target, _foo.Target)
@@ -94,7 +94,7 @@ func TestRequestCache(t *testing.T) {
 	assert.NotNil(t, _foo.ctx)
 	assert.Equal(t, _foo.Context().Err(), nil, "context is still valid")
 	assert.True(t, ok)
-	_, ok = p.svr.getRequest("zed", "")
+	_, ok = p.svr.getRequest("zed")
 	assert.False(t, ok)
 	p.svr.closeRequest(fh)
 	assert.Equal(t, _foo.Context().Err(), context.Canceled, "context is now canceled")
diff --git a/request.go b/request.go
@@ -206,7 +206,6 @@ func (r *Request) opendir(h Handlers, pkt requestPacket) responsePacket {
 // wrap FileReader handler
 func fileget(h FileReader, r *Request, pkt requestPacket) responsePacket {
 	//fmt.Println("fileget", r)
-	var err error
 	r.state.RLock()
 	reader := r.state.readerAt
 	r.state.RUnlock()
@@ -231,7 +230,6 @@ func fileget(h FileReader, r *Request, pkt requestPacket) responsePacket {
 // wrap FileWriter handler
 func fileput(h FileWriter, r *Request, pkt requestPacket) responsePacket {
 	//fmt.Println("fileput", r)
-	var err error
 	r.state.RLock()
 	writer := r.state.writerAt
 	r.state.RUnlock()
@@ -240,7 +238,7 @@ func fileput(h FileWriter, r *Request, pkt requestPacket) responsePacket {
 	}
 
 	data, offset, _ := packetData(pkt)
-	_, err = writer.WriteAt(data, offset)
+	_, err := writer.WriteAt(data, offset)
 	return statusFromError(pkt, err)
 }
 
