producer/consumer: clean up logging races

[judwhite]

producer: log method has potential for edge case race condition
(checking nil then using); logger/logLvl could be set anytime, added
Lock in SetLogger, RLock in log and connect methods
producer: remove unnecessary continue in transactionCleanup for select

[mreiferson]

Hi @judwhite thanks for finding this and the proposed fix. I'd prefer to use atomic.Load/StorePointer rather than locks, feels a bit heavy as written.

[judwhite]

@mreiferson if we do that we should include logLvl for the (admittedly edge case) scenario where a partial write could be read from that address. we'll also have to accept that logger and logLvl may be out of sync for a brief moment if in the middle of a call to SetLogger and log is called - the uglier one is if NewConn is called and they're out of sync.

this could also be solved by including the logger/logLvl in the sig of NewProducer, drop SetLogger and not add any locks/atomics - though it's a breaking API change, an annoyance to all for a theoretical issue.

you could also do nothing and add a comment that SetLogger is expected to be called once during initialization. it keeps the code less complicated and documents assumptions.

let me know your thoughts and I'll update.

[mreiferson]

Ok, you've sold me on this approach. I've got a separate code comment 😁

[mreiferson]

let's wrap logGuard around a smaller critical section (without a defer) and store the w.logger in a local var for passing it along into the new connection instantiation

[mreiferson]

Do we have the same race condition on Consumer?

[judwhite]

@mreiferson what do you think about this approach? if you like it I'll do the same for Conn and Consumer. the intent is to isolate the scope of the critical sections and provide a safe way to read/write logger/logLvl.

[mreiferson]

even better, nice work 👍

[judwhite]

@mreiferson ready for review

[mreiferson]

cool LGTM

mind squashing commits down?

[judwhite]

will do