support concurrent-safe WriteSyncer

[flisky]

Follows up #106

[mention-bot]

@flisky, thanks for your PR! By analyzing the history of the files in this pull request, we identified @akshayjshah and @suyash to be potential reviewers.

[flisky]

Travis errors with "Update failed for go.uber.org/atomic: Cannot detect VCS"

Trigger again.

[flisky]

The CI should pass. Please take a review.

[suyash]

Any chance you could provide a sample program demonstrating usage for this feature?

[flisky]

Actually, I need the lock to do an application-level log rotate, but zap doesn't expose the underlying lockedWriteSyncer, and zap will lock the sink in anyway even if the writesyncer already does.

[akshayjshah]

@flisky I'm sorry, I'm still confused. Can you explain your use case in more detail?

If you're trying to provide your own WriteSyncer that's already locked and handles log rotation, this change isn't necessary. Pass your implementation to zap.New - this library doesn't do anything by default unless you use the config struct.

[flisky]

Pass your implementation to zap.New - this library doesn't do anything by default unless you use the config struct.

It calls the writeSyncer with a zapcore.Lock in my understanding. And zapcore.Lock forcely adds a lock.

#106 said -

In future, we may want to expose a way of setting a WriteSyncer that
already supports concurrent calls

And this is a way trying to advise Lock - if the ws is concurrent safe, use it; if not, wrap ws with our default implementation. But now, the logic is - if the ws is our default concurrent safe instance, use it; if not, wrap it to our default concurrent safe instance.

Is my understanding correct? Thanks!

[akshayjshah]

Oh dear, you're right - I was so sure that we'd already removed that. Let's fix this properly and just remove the automatic lock in NewCore.

The zapcore package shouldn't be supplying default behaviors - it should assume that the programmer knows what she's doing. This is a holdover from a much, much earlier version of this package.

[jcorbin]

It'd be even better if the test uses a concrete type separately implemented from the internal lockedWriteSyncer since, as I understand it, the entire point of this PR is to support external implementations (and avoid double-locking them).

[flisky]

Yes, half part. It also makes sure the internal lockedWriteSyncer is okay.
A separately implementation would be almost same as our default one, so I don't think it necessary.
Would some rewords help? Any thoughts?

[flisky]

Could you give me a final okay to get rid of zapcore.Lock, @akshayjshah & @jcorbin ? Because it's an incompatible change.

If so, would we keep the Lock implementation and just not call it ourselves, or remove it clearly? I'm glad to send another PR:)

[akshayjshah]

We shouldn't get rid of zapcore.Lock, but we should get rid of the automatic use of it in zapcore.NewCore.

[seifer]

Hi, what about this PR? It's a very useful. When will it be merged?

[flisky]

#369 as you wish, @akshayjshah.

[akshayjshah]

@seifer This PR won't be merged, but @flisky was kind enough to open a smaller PR that accomplishes the same thing.