v2: client: Close race conditions

[resolutecake]

To avoid spurious errors there needs to be an atomic.Bool that is set to true immediately on invocation of Close everything that gets a read or write error then need to check that atomic AND errors.Is(err, net.ErrClosed)

Also use fmt.Errorf with %w directive, so that underlying error remains in error chain — that way consumers can also check for net.ErrClosed

failure probability now 50%

What happens is on Close ErrClosed occurs and that error is returned by Close The error occurs in [Client.readResponse] "in response: cannot read tag: %v"

instead of using locks, one can use a clever set of atomics and single-invocation-enforcing atomic.Bool this makes Close idempotent and the code reliable check this out: https://github.com/haraldrudell/parl/blob/main/phttp/http.go it is a bit more complicated for TLS tcp, but can be done in a day or so

the only place where you need lock is Shutdown, because there threads wait. sync.Once

I would also refrain from deferred function literals whenever those are used it means your object model is poor, functions and their argument lists too long

You’d be well served to separate socket management from socket use, ie. separate tcp Listen from imap Login

[emersion]

Please open issues about actual bugs, not about a list of things you would've done differently.

errors.Is(err, net.ErrClosed)

Is this about https://github.com/emersion/go-imap/pull/563?

Also use fmt.Errorf with %w directive, so that underlying error remains in error chain — that way consumers can also check for net.ErrClosed

No: using %w makes the underlying errors part of the API. This should only be done on a case by case basis.

instead of using locks, one can use a clever set of atomics and single-invocation-enforcing atomic.Bool

No: I prefer simple over clever.

I would also refrain from deferred function literals

No, I don't believe that's the case.

You’d be well served to separate socket management from socket use, ie. separate tcp Listen from imap Login

Doesn't make sense to me.

[resolutecake]

It is not the same issue. It is during login. imapclient appears to be closing the socket while it is writing resulting in short write

I wrote a stub imap client that logs in just fine with the exact same login request

• OK Gimap ready for requests…

I am looking into it.

This would be simpler if each request had its own struct type with fields containing all possible parameters then a single method that sends serialized data to Write. Like Google did with route message: https://cs.opensource.google/go/x/net/+/refs/tags/v0.19.0:route/route.go;l=72

[emersion]

each request had its own struct type

This would disallow streaming of literals.

[resolutecake]

I verified that the issue is not that the socket is closed

There are 4 copies of the socket, and somewhere in that, bufio encounters a short write although the socket is OK — gmail fails — my own imap server works but the short write error is visible in the logs

I’m thinking I can catch it with the debugger

I’ll have it figured by tomorrow

—

Writing functional chaining can make you feel good It is not a common way to write Go code Go is a language of structs. If you have more than one value, you use a struct or will eventually have to refactor

I return read-to-end sockets for Android using that Google architecture What I realized was that people who don’t use that package still use the struct because it can configure routes on any operating system. They put things in the struct, get the byte-stream and use it for something else

Baseless assertions isn’t gong to change that Google has shown us how to do things

Another new language feature is those locks used to retrieve multiple fields could be atomic.Pointer returning an updatable read-only struct of any number of fields thread-safe at once without a lock

What has happened is that any field written after the new function often becomes atomic.Pointer

Anyway, I just need this imapclient to work, and right now it doesn’t