Confusion around Wait() function

[freeformz]

Confusion around Wait() function

[freeformz]

Example: I can get this test to reliably panic:

=== RUN   TestAsyncTransportPanic
panic: sync: WaitGroup is reused before previous Wait has returned

func TestAsyncTransportPanic(t *testing.T) {
	transport := rollbar.NewAsyncTransport("", "", 1)
	transport.SetLogger(&rollbar.SilentClientLogger{})
	body := map[string]interface{}{
		"hello": "world",
	}
	started := make(chan struct{})
	go func() {
		close(started)
		for {
			transport.Wait()
		}
	}()
	iter := make([]struct{}, 100)
	<-started
	for range iter {
		result := transport.Send(body)
		if result != nil {
			t.Error("Send returned an unexpected error:", result)
		}
	}
}

While this test doesn't use a webserver it does emulate Sending happening from one goroutine and Waits happening in another goroutine.

[waltjones]

@freeformz Thank you for the analysis and the test case. I want to review your comments thoroughly before responding. It will probably be next week.

[freeformz]

@waltjones Thanks. I'm happy to update my branch (if possible) once the intent of Wait() is hashed out. I see some options, all of which change the API a bunch however:

• Transport's Wait() is removed. Close() instead waits up to some amount of time for the channel to be drained. This implies that WrapAndWait (and similar) Client functions would go away unless...
• Transport's Send is modified to return a function that when called will wait for the message enqueued by that Send call to be sent to rollbar (or dropped on the floor). The Sync transport would just return a no-op func.

There are probably others, but they really depend on Wait()'s intent.

FWIW: I have most of the first thing done in my branch and don't think it would take more than an evening or so to add the second one.

[waltjones]

@freeformz I think the semantics aren't very clear, either in the doc or in the code. And I agree that Add() and Wait() need to be synchronized.

Close() already does call Wait() after closing the channel, so to that extent your first bullet is already implemented. The doc for Close() says "Close is an alias for Wait", but that's not even loosely true. While it is possible to continue to use the transport after Wait() has completed, the transport cannot be used again after Close().

The purpose of Wait() in any of the Rollbar SDKs is to allow network requests to complete before program exit. (Or thread exit if that is applicable.) For the Go SDK, the most straightforward usage would be to Close() at the end of main() and after all goroutines have exited. Or if not waiting for all goroutines, Wait() would be a safe alternative assuming the necessary synchronization was there.

My understanding is, it's safe to not close the channel. So Wait() could be used in all cases, and Close() isn't strictly needed.

Does that sound right to you? Is there anything you'd add to that?

(BTW: I needed to separate my personal and work github accounts for a few different reasons; this is my work account).

What about the other Wait functions and functions that take a wait bool (like WrapAndWait, WrapWithArgs, LogPanic, etc)? These end up calling Wait(), but it really doesn't work in the way it's used.

My understanding is, it's safe to not close the channel. So Wait() could be used in all cases, and Close() isn't strictly needed.

I don't think I understand that. The way Wait() is currently implemented isn't really waiting for anything specific though. It's entirely possible for a Wait() call to return immediately after an Add(), or block almost forever. It all depends on behavior that isn't specified and that is what is problematic.

Also, the semantics of the way the functions are implemented seem to indicate that Wait is to be used in 2 ways. The first would be the way you described, the second would be Add() then Wait() for it to be sent to rollbar. ATM, neither of those scenarios work as expected.