Add ability to handle context cancellations for TCP protocol

[tinybit]

Summary

Issue:

• There's has no way to cancel a running query using context's cancel functions. #1388

Checklist

Delete items not relevant to your PR:

• Unit and integration tests covering the common scenarios were added
• A human-readable description of the changes was provided to include in CHANGELOG
• For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

[jkaflik]

LGTM. I will do a small testing on my side and then we can merge.

[jkaflik]

Is it something you found during debugging or presumably want to safe?

[tinybit]

yup

[tinybit]

hmm, this is outdated code, please check the latest changes
https://github.com/ClickHouse/clickhouse-go/pull/1389/files

[tinybit]

also, safety is not presumed, it's necessary here.
previously, in conn_process.go access to c.close and c.buffer was single-threaded.
now i'm reading data in background goroutine and in foreground thread i'm waiting on select:

// do reads in background
errCh := make(chan error, 1)
doneCh := make(chan bool, 1)

go func() {
	err := c.processImpl(ctx, on) // this accesses c.reader internally (reads data)
	if err != nil {
		errCh <- err
		return
	}

	doneCh <- true
}()

// select on context or read channel (errors)
select {
case <-ctx.Done():
	c.cancel() // calls c.close(), it accesses c.reader internally (sets c.reader to nil)
	return ctx.Err()

case err := <-errCh:
	return err

case <-doneCh:
	return nil
}

1. Closing connections must be done very carefully. Currently conn.Close() is broken because multiple calls to conn.Close() have issues deadlocking the thread it was called from

2. Also multiple calls to c.close() from multiple threads is not thread safe, doing so will result in data races.

So, this needed to be fixed from this:

func (c *connect) close() error {
    if c.closed {
        return nil
    }
    c.closed = true
<...>

To a thread-safe version of it

func (c *connect) close() error {
	c.closeMutex.Lock()
	if c.closed {
		c.closeMutex.Unlock()
		return nil
	}
	c.closed = true
	c.closeMutex.Unlock()
<...>

This guarantees that under no circumstances c.close() can be called twice no matter what.

3. Besides this we need to guarantee threadsafe access to c.reader from c.processImpl(), c.cancel() and c.close(), hence I've added c.readerMutex()

4. One more thing: the order of operations during closing a connection:
   Old version:

func (c *connect) close() error {
	c.closeMutex.Lock()
	if c.closed {
		c.closeMutex.Unlock()
		return nil
	}
	c.closed = true
	c.closeMutex.Unlock()

	c.buffer = nil

	c.readerMutex.Lock()
	c.reader = nil
	c.readerMutex.Unlock()

	if err := c.conn.Close(); err != nil {
		return err
	}
	
	return nil
}

As you can see here, we're changing c.buffer and c.reader that might be currently reading/sending data, before we close the network connection. Connection must be closed first, in order to stop any i/o operations on c.buffer and c.reader (and related contexts), then it's safe to deallocale c.buffer and c.reader.

func (c *connect) close() error {
	c.closeMutex.Lock()
	if c.closed {
		c.closeMutex.Unlock()
		return nil
	}
	c.closed = true
	c.closeMutex.Unlock()

	if err := c.conn.Close(); err != nil {
		return err
	}

	c.buffer = nil

	c.readerMutex.Lock()
	c.reader = nil
	c.readerMutex.Unlock()

	
	return nil
}

[jkaflik]

do you think it makes sense to name it explicit?

Suggested change

	mutex sync.Mutex
	readerMutex sync.Mutex

[tinybit]

yeah, good idea

[tinybit]

done

[jkaflik]

is it something you found when while debugging or only added for safety?

[tinybit]

hmm, this is outdated code, please check the latest changes
https://github.com/ClickHouse/clickhouse-go/pull/1389/files

it was all found during debugging, yes.

note: if c.buffer == nil { is already removed in latest commit, it was incorrect assumption on my side, trying to solve synchronisation issues.

if c.isClosed() { check is necessary. We can't perform read/send operations on closed connections

[jkaflik]

I conducted basic tests. This will be merged with next minor release in two weeks.