Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/thread safe logging #139

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c7f0675
feat(Main): added finalise and finalised
mcakircali Jun 20, 2024
0407bb1
feat(Channel): introduce EmptyChannel
mcakircali Jun 20, 2024
d9c9862
fix(ChannelBuffer): rename OutputChannel
mcakircali Jun 20, 2024
d4825ae
fix(ChannelBuffer): sync() in Channel not in dtor here
mcakircali Jun 20, 2024
2d73e73
fix(Log): set missing private ctor/dtor = default
mcakircali Jun 20, 2024
82a17f5
fix(Log): thread and lifetime safe Channels
mcakircali Jun 20, 2024
60c5399
fix(Library): renamed Channel => OutputChannel
mcakircali Jun 20, 2024
225e43a
fix(Library): log channels are thread (and lifetime) safe
mcakircali Jun 20, 2024
88e5eed
fix(Test): finalise the Main object
mcakircali Jun 20, 2024
0faf773
fix(Test): renamed Channel to OutputChannel
mcakircali Jun 20, 2024
4e59bf0
feat(Test): added multi-threaded log testing
mcakircali Jun 20, 2024
ae98034
fix(Library): revert unintended changes
mcakircali Jun 20, 2024
346a23c
fix(Channel): revert inheritance and restore interface
mcakircali Jun 20, 2024
0519c53
feat(ChannelBuffer): added VoidBuffer for EmptyChannel
mcakircali Jun 20, 2024
5dd813f
refactor(Channel): rename OutputChannel to Channel
mcakircali Jun 20, 2024
3459dde
fix(Main): set finished to false on initialise
mcakircali Jun 20, 2024
1e89c86
chore(Test): relocated global tester object
mcakircali Jun 20, 2024
7c3c1bc
fix(Channel): rather override, and add missing stream operator overload
mcakircali Jun 20, 2024
6ddfc94
fix(Log): make sure no copy nor default ctor
mcakircali Jun 20, 2024
c948b30
fix(Main): typo UK
mcakircali Jun 20, 2024
bfdce18
refactor(VoidBuffer): moved detail to source
mcakircali Jun 28, 2024
8444660
fix(Log): includes
mcakircali Jun 28, 2024
25fac98
fix(Library): debug channel as member, empty channel as thread_local
mcakircali Jun 28, 2024
2bf8c5e
fix(Log): cleanup
mcakircali Jun 28, 2024
0d49f7a
fix(Main): finalise Main (any child) via std::atexit
mcakircali Jun 28, 2024
3f8b957
test(Log): cleanup
mcakircali Jun 28, 2024
0ba7337
test(Log): add worse case
mcakircali Jul 1, 2024
1968f5d
test(Log): comment out the worse case
mcakircali Sep 10, 2024
86ec904
Merge branch 'develop' into feature/thread-safe-logging
mcakircali Sep 10, 2024
8caa48e
Add comments to note about possible problems
geier1993 Sep 11, 2024
19507a3
refactor(logging): emptyChannel name
mcakircali Sep 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/eckit/log/Channel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,17 @@ namespace eckit {

//----------------------------------------------------------------------------------------------------------------------

Channel::Channel(LogTarget* target) :
std::ostream(new ChannelBuffer()), buffer_(dynamic_cast<ChannelBuffer*>(rdbuf())) {
Channel::Channel(ChannelBuffer* buffer): std::ostream(buffer), buffer_(buffer) { }

Channel::Channel(LogTarget* target): Channel(new ChannelBuffer()) {
ASSERT(buffer_);
if (target) {
buffer_->setTarget(target);
}
// std::cerr << "Channel::Channel()" << std::endl;
if (target) { buffer_->setTarget(target); }
}

Channel::Channel(VoidBuffer* dummy): std::ostream(nullptr), buffer_(dummy) { }

Channel::~Channel() {
// std::cerr << "Channel::~Channel() " << *this << std::endl;
buffer_->sync();
delete buffer_;
}

Expand Down Expand Up @@ -69,7 +68,6 @@ void Channel::addStream(std::ostream& out) {
buffer_->addStream(out);
}


void Channel::setFile(const std::string& path) {
buffer_->setFile(path);
}
Expand All @@ -96,4 +94,8 @@ void Channel::unindent() {

//----------------------------------------------------------------------------------------------------------------------

EmptyChannel::EmptyChannel(): Channel(new VoidBuffer()) { }

//----------------------------------------------------------------------------------------------------------------------

} // namespace eckit
44 changes: 30 additions & 14 deletions src/eckit/log/Channel.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

namespace eckit {

class VoidBuffer;
class ChannelBuffer;
class LogTarget;

Expand All @@ -29,8 +30,7 @@ typedef void (*channel_callback_t)(void* data, const char* msg);

/// Output channel that is an std::ostream but more functional

class Channel : public std::ostream, private NonCopyable {

class Channel: public std::ostream, private NonCopyable {
public: // methods
Channel(LogTarget* = 0);

Expand All @@ -39,7 +39,7 @@ class Channel : public std::ostream, private NonCopyable {
bool operator!() const;
operator bool() const;

void indent(const char* prefix = "");
void indent(const char* space = "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a first glance I disagree with the renaming because It looks like I can use an arbitrary character sequence here while I interpret indentation either as <space> vs <tab> or a numerical value e.g. 5 spaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also disagree with this renaming. Although, that said, it is plausible to use non-space indents (e.g. ">>>" or "...")

void unindent();

void setStream(std::ostream& out);
Expand All @@ -51,40 +51,56 @@ class Channel : public std::ostream, private NonCopyable {
void setCallback(channel_callback_t cb, void* data = 0);
void addCallback(channel_callback_t cb, void* data = 0);

void setTarget(LogTarget*);
void addTarget(LogTarget*);
void setTarget(LogTarget* target);
void addTarget(LogTarget* target);

void reset();

private: // members
protected: // methods
explicit Channel(VoidBuffer* dummy);

private: // methods
explicit Channel(ChannelBuffer* buffer);

virtual void print(std::ostream& s) const;

friend std::ostream& operator<<(std::ostream& os, const Channel& c) {
c.print(os);
return os;
}

void print(std::ostream& s) const;

ChannelBuffer* buffer_;
private: // members
ChannelBuffer* buffer_ {nullptr};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a std::unique_ptr<ChannelBuffer>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently explicitly deleted in the constructor. Please change to unique_ptr. Is a good change.


friend class Log;
};


//----------------------------------------------------------------------------------------------------------------------


class AutoIndent {

Channel& channel_;

public:
AutoIndent(Channel& channel, const char* prefix = "") :
channel_(channel) { channel_.indent(prefix); }
AutoIndent(Channel& channel, const char* prefix = ""): channel_(channel) { channel_.indent(prefix); }
~AutoIndent() { channel_.unindent(); }
};

//----------------------------------------------------------------------------------------------------------------------

class EmptyChannel: public Channel {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two remarks:

1.) This should be named VoidChannel or NullChannel (as in > /dev/null), because it is more interesting that the messages disappear into the void than it is important that the channel is empty.

2.) Is this type actually needed? A VoidChannel could equally be constructed by a factory function, e.g. createVoidChannel.

public: // methods
EmptyChannel();

~EmptyChannel() = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be made virtual with virtual or override


private: // methods
void print(std::ostream& s) const override { s << "EmptyChannel()"; }

friend class Log;
};

//----------------------------------------------------------------------------------------------------------------------

} // namespace eckit

#endif
41 changes: 34 additions & 7 deletions src/eckit/log/ChannelBuffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,25 @@ namespace eckit {

//----------------------------------------------------------------------------------------------------------------------

ChannelBuffer::ChannelBuffer(std::size_t size) : std::streambuf(), target_(0), buffer_(size) {
ASSERT(size);
ChannelBuffer::ChannelBuffer(const std::size_t size): buffer_(size) {
init();
}

void ChannelBuffer::init() {
setp(buffer_.data(), buffer_.data() + buffer_.size());
}

ChannelBuffer::~ChannelBuffer() {
reset();
if (target_) {
target_->detach();
target_ = nullptr;
}
}

bool ChannelBuffer::active() const {
return target_ != 0;
}


void ChannelBuffer::setTarget(LogTarget* target) {
ASSERT(target);

Expand Down Expand Up @@ -72,7 +77,7 @@ bool ChannelBuffer::dumpBuffer() {
// Explicitly check that `pptr()` is not larger than end of buffer. Racecondition can end up adding larger values.
target_->write(buffer_.data(), std::min(pptr(), buffer_.data() + buffer_.size()));
}
setp(buffer_.data(), buffer_.data() + buffer_.size());
init();
return true;
}

Expand Down Expand Up @@ -108,11 +113,11 @@ void ChannelBuffer::setStream(std::ostream& out) {
setTarget(new OStreamTarget(out));
}

void ChannelBuffer::addFile(const std::string& path, size_t bufferSize) {
void ChannelBuffer::addFile(const std::string& path, const std::size_t bufferSize) {
setTarget(new TeeTarget(target_, new FileTarget(path, bufferSize)));
}

void ChannelBuffer::setFile(const std::string& path, size_t bufferSize) {
void ChannelBuffer::setFile(const std::string& path, const std::size_t bufferSize) {
setTarget(new FileTarget(path, bufferSize));
}

Expand Down Expand Up @@ -144,4 +149,26 @@ void ChannelBuffer::print(std::ostream& s) const {

//----------------------------------------------------------------------------------------------------------------------

VoidBuffer::VoidBuffer(): ChannelBuffer(0) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be defaulted


VoidBuffer::~VoidBuffer() = default;

bool VoidBuffer::dumpBuffer() {
return true;
}

std::streambuf::int_type VoidBuffer::overflow(std::streambuf::int_type ch) {
return ch;
}

std::streambuf::int_type VoidBuffer::sync() {
return 0;
}

void VoidBuffer::print(std::ostream& os) const {
os << "VoidBuffer";
}

//----------------------------------------------------------------------------------------------------------------------

} // namespace eckit
57 changes: 42 additions & 15 deletions src/eckit/log/ChannelBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
#ifndef eckit_log_ChannelBuffer_h
#define eckit_log_ChannelBuffer_h

#include <cstddef>
#include <ostream>
#include <streambuf>
#include <string>
#include <vector>

#include "eckit/log/Channel.h"
Expand All @@ -28,15 +31,12 @@ namespace eckit {

class LogTarget;

/// Stream buffer to be usedby Channel
/// Stream buffer to be used by Channel
class ChannelBuffer : public std::streambuf, private NonCopyable {
private: // types
static constexpr const std::size_t DEFAULT_SIZE = 1024;

private: // methods
/// constructor, taking ownership of stream
ChannelBuffer(std::size_t size = 1024);

~ChannelBuffer() override;

bool active() const;

void reset();
Expand All @@ -50,13 +50,19 @@ class ChannelBuffer : public std::streambuf, private NonCopyable {
void setStream(std::ostream& out);
void addStream(std::ostream& out);

void setFile(const std::string& path, size_t bufferSize = 4 * 1024);
void addFile(const std::string& path, size_t bufferSize = 4 * 1024);
void setFile(const std::string& path, std::size_t bufferSize = 4 * 1024);
void addFile(const std::string& path, std::size_t bufferSize = 4 * 1024);

void setCallback(channel_callback_t cb, void* data = 0);
void addCallback(channel_callback_t cb, void* data = 0);

protected: // methods
ChannelBuffer(std::size_t size = DEFAULT_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in favor of this change because I believe this to be going into the wrong direction. Ideally Channel is not constructing a ChannelBuffer in its constructor but instead getting configured with anything satisfying the ChannelBuffer interface, aka dependency injection for testing.


~ChannelBuffer() override;

void init();

/// override this to change buffer behavior
/// @returns true if no error occured
virtual bool dumpBuffer();
Expand All @@ -69,24 +75,45 @@ class ChannelBuffer : public std::streambuf, private NonCopyable {
/// @see dumpBuffer
int_type sync() override;

protected: // members
LogTarget* target_;

std::vector<char> buffer_;

private:
friend std::ostream& operator<<(std::ostream& os, const ChannelBuffer& c) {
c.print(os);
return os;
}

void print(std::ostream& s) const;
virtual void print(std::ostream& s) const;

protected: // members
LogTarget* target_ {nullptr};

std::vector<char> buffer_;

friend class Channel;
};

//----------------------------------------------------------------------------------------------------------------------

/// Channel buffer that voidify output streams
class VoidBuffer: public ChannelBuffer {
private: // methods
VoidBuffer();

~VoidBuffer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be virtual / override as it is polymorphically destroyed from Channel dtor


protected: // methods
bool dumpBuffer() override;

int_type overflow(int_type ch) override;

int_type sync() override;

private: // methods
void print(std::ostream& os) const override;

friend class EmptyChannel;
};

//----------------------------------------------------------------------------------------------------------------------

} // namespace eckit

#endif
Loading
Loading