After the RequestOpenChannel changes, it's now possible to specify the
name and message of an outbound request as variables of the channel handler,
instead implementing an interface method to return them.
Also added the SendResponse method, which is necessary to respond to an
inbound request that was in the Pending state.
There were a few related issues with ProcessAuthAsClient/Server that
could cause deadlocks or leak goroutines:
Break() could end up being called more than once, which is always a
deadlock. This is fixed by using a sync.Once.
RequestOpenChannel was called without Do, and was called before
Process in the same goroutine, which would have deadlocked if Do was
used.
Timing out the authentication attempt wouldn't directly abort
Process(); it would only exit if the connection was closed somewhere
else.
It may be a good idea to change some of this to guarantee that
ProcessAuthAsClient returns either an authenticated connection or closes
the connection, but I'll leave that as a separate task for the moment.
After the last round of fixes in Do, there was still one major issue
making it impossible to use Do or Break safely: closing connections.
Connections can be closed spontaneously, and this causes Process to
return. At that moment any ongoing call to Do or Break has deadlocked;
nothing will ever read those channels again. To prevent this, we have to
ensure that Do or Break won't try to send to Process' channels once it
has stopped reading from them. Doing that without other races is tricky.
The solution here, briefly, is to acquire a mutex in Do and Break before
checking the `closed` boolean, and hold that mutex for the entire
operation (including any blocking channel operations). When Process is
closing down the connection, it uses a separate goroutine to acquire the
same mutex and change the boolean, while still handling channel reads.
Once the boolean has changed, the mutex guarantees that nothing will try
to send to these channels again.
I've tried to document the problems and solutions in the code, because
it is subtle in some places and this is definitely critical code.
There were several issues with the Do function that made it nearly
impossible to write safe code.
First, Do cannot be called recursively -- it will deadlock. There is
actually no way to implement a safe and recursive Do (or mutex) in Go,
because there is no primitive that will identify the current goroutine.
RequestOpenChannel used Do internally, which made it impossible to open
channels safely in many circumstances. That has been removed, so all
calls to RequestOpenChannel must be changed to happen under Do now.
Do now has more documentation and a new rule: no code exposed through
API can use Do, unless it has sole custody of the connection (such as
ProcessAuthAsClient).
Related to that problem, Do was impossible to call from inside handlers
(or anything else on the process goroutine) -- it would again just
deadlock. This is resolved by wrapping calls into user code to continue
handling invocations of Do (and only those) while the handler is
executing.
There is a third issue with connection close, but it will be addressed
in a separate commit
And finally, because it's impossible to timeout or interrupt a call to
Do, I also added a DoContext method that takes a go Context, which is
also passed through to the called function.
This fixes a quirk where it would've been difficult to tell which of
several channels of the same type+direction is the one you just created.
It's also a fairly common pattern to want to interact with a channel
right after opening it; for example, a chat channel is opened and can
immediately send messages before getting the peer response. It's
convenient to not have to do a separate lookup.
RequestOpenChannel is the primary API to open a new outbound channel. It
was written to take a connection.Handler and use OnOpenChannelRequest
to get a channels.Handler to represent the new channel, which is the
same path that inbound channels will take.
Going through the global OnOpenChannelRequest method makes this much
less flexible and prevents passing parameters to the new channel handler
during creation. This also requires users of the API to know/find the
connection handler, or worse, to boilerplate one into existence for their
channel creation.
Instead, I think this function should take a channels.Handler directly,
so that the caller gets full control over the handler for their new
channel.
As part of that change, I've also moved the authentication logic in
AutoConnectionHandler to be contained entirely within
{In,Out}boundConnectionHandler.
ChatChannel didn't return the message ID for sent messages, which made
using the returned ACKs impossible. The SendMessage method now returns
the uin32 messageID.
Also, SendMessage didn't support the TimeDelta field for messages, which
is used for queued or resent messages. This is now available as
SendMessageWithTime.
And finally, the ChatMessageAck callback didn't indicate if mesasges
were accepted or not, which is part of the protocol. That was added as a
field, which is unfortunately a breaking API change, but I've made
enough of those lately to not feel guilty about it.
There are few situations where a pointer to an interface is useful in
Go, and this isn't one. Interfaces can hold types by value or pointer,
so long as that type fulfills the interface.
This is needed to allow creating a Connection for an arbitrary
application-provided io.ReadWriteCloser, instead of doing network logic
inside of go-ricochet. ricochet-go does its own connection management.
I would rather rework this API so that Connection has two construct
methods, inbound and outbound, that do version negotiation and are
always used. The methods that do networking and construct a Connection
would then be a separate (and non-root) package building on top of that.
Sarah Jamie Lewis
Sarah Jamie Lewis
John Brooks
Dan Ballard