Calls to Write() after calling Close() in sync mode reopen the connection

[akerouanton]

Currently, in sync mode, the code reopens the connection when Write() is called after Close(). This could happen either because:

1. Client's code call these functions serially from a single goroutine ;
2. Or when two goroutines call these functions concurrently and the connection mutex lead to both goroutines being serialized in that specific order ⬆️

In the first case, if Write() is serially called after Close(), it might seem legit to have to call Close() once again but in the second case, it doesn't seem good to have to call Close() multiple times to be really sure the connection is closed.

This case doesn't happen in async mode because f.Close() takes care of setting f.chanClosed = true and f.appendBuffer() checks if f.chanClosed = true and returns an error if that's the case.

fluent-logger-golang/fluent/fluent.go

Lines 350 to 357 in c16ce5d

	func (f *Fluent) Close() (err error) {
	if f.Config.Async {
	f.pendingMutex.Lock()
	if f.chanClosed {
	f.pendingMutex.Unlock()
	return nil
	}
	f.chanClosed = true

fluent-logger-golang/fluent/fluent.go

Lines 391 to 396 in c16ce5d

	func (f *Fluent) appendBuffer(msg *msgToSend) error {
	f.pendingMutex.RLock()
	defer f.pendingMutex.RUnlock()
	if f.chanClosed {
	return fmt.Errorf("fluent#appendBuffer: Logger already closed")
	}

I suggest to change the code for sync mode to not accept any new messages when f.Close() has been called and document that change (as well as thread safety improvement done in #82).

I've written the following test to confirm my findings (although I'm not sure how to test the inverse behavior). I ran it against the current master branch and on pre-#82 code to make sure this behavior wasn't introduced by that change. Both tests trigger the error.

func TestSyncWriteAfterCloseFails(t *testing.T) {
	d := newTestDialer()

	go func() {
		var f *Fluent
		var err error
		if f, err = newWithDialer(Config{
			Async: false,
		}, d); err != nil {
			t.Errorf("Unexpected error: %v", err)
		}

		_ = f.PostWithTime("tag_name", time.Unix(1482493046, 0), map[string]string{"foo": "bar"})

		if err := f.Close(); err != nil {
			t.Errorf("Unexpected error: %v", err)
		}

		_ = f.PostWithTime("tag_name", time.Unix(1482493050, 0), map[string]string{"fluentd": "is awesome"})
	}()

	conn := d.waitForNextDialing(true, false)
	conn.waitForNextWrite(true, "")

	conn = d.waitForNextDialing(true, false)
	conn.waitForNextWrite(true, "")

        // isOpen is a field added to Conn specifically for this test
	if conn.isOpen == true {
		t.Error("Connection has been reopened.") // This is currently triggered
	}
}

[tagomoris]

@fluent/team-golang Could someone check this idea?

[fujimotos]

@tagomoris I'll take alook at this ticket this evening.

[fujimotos]

In the first case, if Write() is serially called after Close(), it might seem legit to have to call Close() once again but in the second case, it doesn't seem good to have to call Close() multiple times to be really sure the connection is closed.

@akerouanton So your suggestion is that an explicit call to
Fluent.Close() should behave like POSIX's close(2) -- i.e. should make
the subsequent write() calls fail, right?

In a design wise, I don't have any particular opposition to that proposal;
It indeed seems to be a more consistent behaviour.

I suggest to change the code for sync mode to not accept any new messages when f.Close() has been called

I'm thinking that probably the most streight forward way to implement
your proposal is to reuse the chancClose flag.

We can set it to true in Fluent.Close() and add a simple flag check to
this line in postRawData():

https://github.com/fluent/fluent-logger-golang/blob/master/fluent/fluent.go#L277

But do you have any suggestion on how it should be implemented?

[akerouanton]

So your suggestion is that an explicit call to
Fluent.Close() should behave like POSIX's close(2) -- i.e. should make the subsequent write() calls fail, right?

@fujimotos Yeah, that's it.

I'm thinking that probably the most streight forward way to implement your proposal is to reuse the chancClose flag.

We can set it to true in Fluent.Close() and add a simple flag check to this line in postRawData():

That's what I had in mind 🙂 I'd also rename chanClosed to closed.

[fujimotos]

Resolved by #105.