Thread-safety concerns wrt. handler creation, safe alternative proposal

[ktoso]

The current implementation makes LogHandler a class and then uses:

public struct FileLogHandler: LogHandler {
    private let stream: TextOutputStream
    private var label: String

    // Necessary because the factory method used in the swift logger doesn't allow a throwing constructor.
    public func handler(label: String) -> LogHandler {
        self.label = label // a) not thread-safe 
        return self        // b) semantically this changes all other loggers as well, but it should not (!)
    }

invoked as

        LoggingSystem.bootstrapInternal { label in logFileHandler.handler(label: label) } 

This is not thread-safe. The factory will be called from many threads, and thus many threads will perform writes to the shared variable label -- this is not safe and thus we can't do it like that.

---

The design of the lib has to likely change a little bit; it is rather:

• creating a handle to a file, but it is NOT a log handler yet (!)
  • let allLogs = try FileLogging(to: "all.log")
• then this can be used to create logHandlers, like so:

  •     `LoggingSystem.bootstrap(allLogs.handler)`
    

the handler function should be:

// struct FileLogging { 
    public func handler(label: String) -> FileLogHandler

or something like that -- note that we create a new log handler rather than mutating anything shared; this way logging will work fine; each Logger(label:) will get the right label based handler, and we avoid any concurrency issues as well.

WDYT @crspybits ?

[crspybits]

Good catch. I'm thinking I'll need to do:

    public func handler(label: String) -> FileLogHandler {
        return try! FileLogHandler(label: label, localFile: self.localFile)
    }

Is that about right?

This lack of throwing methods is difficult. I don't like seeing the force unwrapping.

[ktoso]

I mean to put the throwing part into the first bit:

let all = try FileLogging(to: "all.log")

and then the handler part does not need to throw because it does not make any new file stuff, it only points to the one you opened before:

    public func handler(label: String) -> FileLogHandler {
        return FileLogHandler(label: label, localFile: self.localFile)
    }

Should work unless I'm missing something?

---

The rationale here is that if we made creating handlers throwing... then all Logger(label: "hello") would have to become throwing as well really, and that makes for a bad end-user experience.

IMHO "setup" should throw early, but once the system is ready there's no need to throw -- you already have access to the target file etc.

[crspybits]

I think I'm understanding now. I made another commit. See how this looks? Thanks!

[crspybits]

Closing this as I think it's done.