go-jmap: client: lock mutex after calling Authenticate when uploading/downloading v1 PROPOSED

Silvan Jegen: 1
 client: lock mutex after calling Authenticate when uploading/downloading

 1 files changed, 4 insertions(+), 0 deletions(-)

In JMAP, each request returns the state string of the session object. I've found
it to be pretty good practice to check that on every request against the session

Silvan Jegen <me@sillymon.ch>

3 months ago

I wasn't aware of this state string! I have read up a bit about it and
from what I understand, some of the API response objects also return
the session state, which then can be used to "... quickly determine if
the session information has changed (e.g., an account has been added or
removed), so they need to refetch the object."[0]

The documentation mentions that "If the value of any other property
on the Session object changes, this string will change.". To me this
sounds like this state shouldn't change often, since the session object
contains mostly long-lived fields (like the capabilities of the server,
for example).

Next

object you have, and if it's different, request a new session object. In this
case, that would be to call Authenticate.

If we remove that method, the only means is to then create a *new* client. This
has *some* performance hit because the old client would have been maintaining
the connections - while a new one would have to renegotiate TLS, etc.

"Tim Culverhouse" <tim@timculverhouse.com> wrote:

"Tim Culverhouse" <tim@timculverhouse.com> wrote:

[PATCH go-jmap] client: lock mutex after calling Authenticate when uploading/downloading Export this patch

This makes sure we don't end up Unlocking an unlocked mutex while
also covering the critical section where we access the Session's
Upload/Download URLs.

Silvan Jegen <me@sillymon.ch>

3 months ago

AFAICT the locking dance we have to do in the client is only necessary
because we allow Authenticate to be called in so many places.

I wonder if it wouldn't be better to just introduce a `NewClient()
(*Client, error)` function that returns an authenticated client with a
valid session (or an error).

This would allow us to get rid of all the locking since the session will
always be there, valid and read-only.

It would be a breaking change and it would mean that you could not create
an unauthenticated client. I don't think these are big issues though
since the module is not yet at version 1 (so no backwards compatibility
is expected) and an unauthenticated client is good for nothing anyways
from what I can tell.

Tim Culverhouse <tim@timculverhouse.com>

3 months ago

I don't see it as a big issue either, AFAIK there are only a few users of the
library and it wouldn't be too difficult of a change.

Next

What do you think?

---
 client.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/client.go b/client.go
index 33aa553..b0c461f 100644
--- a/client.go
+++ b/client.go
@@ -192,6 +192,8 @@ func (c *Client) UploadWithContext(
 		if err != nil {
 			return nil, err
 		}
+
+		c.Lock()
 	}
 
 	url := strings.ReplaceAll(c.Session.UploadURL, "{accountId}", string(accountID))
@@ -246,6 +248,8 @@ func (c *Client) DownloadWithContext(
 		if err != nil {
 			return nil, err
 		}
+
+		c.Lock()
 	}
 
 	urlRepl := strings.NewReplacer(
-- 
2.47.1

Silvan Jegen <me@sillymon.ch>

3 months ago

Silvan Jegen <s.jegen@gmail.com> wrote: