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

Already on GitHub? Sign in to your account

Clean streams #6

Merged
merged 4 commits into from Aug 16, 2012

Conversation

Projects
None yet
2 participants
Contributor

erikfrey commented Aug 16, 2012

@nova77 after reviewing the code I agree that boost::optional isn't really working. It was forcing me to explicitly call reset() all over the handler. Instead I've changed the stream classes to have a more explicit opened/closed state, and when streams are destroyed the naturally close. This fits better with the model that when a conn hangs up, the dtor cleans up.

I also cleaned up some nomenclature and some of the comments in the queue class.

erikfrey added some commits Aug 16, 2012

@erikfrey erikfrey clean up iqstream/oqstream semantics
- no more boost::optional
- explicit opened/closed state for both streams
- stream dtors handle closing/aborting
- clearer documentation of queue class
25f9e24
@erikfrey erikfrey update handler to use new iqstream/oqstream 287e60a

@nova77 nova77 commented on the diff Aug 16, 2012

include/darner/queue/queue.h
@@ -34,15 +32,17 @@ class queue
{
public:
+ // a queue can only ever have a backlog of 2^64 items. so at darner's current peak throughput you can only run the
+ // server for 23 million years :(
@nova77

nova77 Aug 16, 2012

Contributor

Y23M!!

@nova77 nova77 commented on an outdated diff Aug 16, 2012

include/darner/util/queue_map.hpp
public:
- typedef boost::ptr_map<std::string, queue>::iterator iterator;
- typedef boost::ptr_map<std::string, queue>::const_iterator const_iterator;
+ typedef std::map<std::string, boost::shared_ptr<queue> >::iterator iterator;
+ typedef std::map<std::string, boost::shared_ptr<queue> >::const_iterator const_iterator;
@nova77

nova77 Aug 16, 2012

Contributor

For consistency?

typedef container_type::iterator iterator;
typedef container_type::const_iterator const_iterator;

@nova77 nova77 commented on the diff Aug 16, 2012

src/net/handler.cpp
- async_read_until(
- socket_, in_, '\n',
- bind(&handler::parse_request, shared_from_this(), _1, _2));
+ async_read_until(socket_, in_, '\n', bind(&handler::parse_request, shared_from_this(), _1, _2));
@nova77

nova77 Aug 16, 2012

Contributor

I thought your terminator was \r\n. Would it make a difference?

@erikfrey

erikfrey Aug 16, 2012

Contributor

well read_until's easiest signature is to just match a single char. i'm actually being a bit permissive here on purpose. kestrel also doesn't exactly follow the spec here - it's to allow people to netcat in and run commands in os's that don't do the \r.

@nova77

nova77 Aug 16, 2012

Contributor

Yeah, it makes sense. You have my blessing. ;)

@nova77 nova77 and 1 other commented on an outdated diff Aug 16, 2012

src/net/handler.cpp
@@ -92,7 +84,8 @@ void handler::write_stats()
void handler::write_version()
{
- done(true, "VERSION " + string(DARNER_VERSION) + "\r\n");
+ buf_ = "VERSION " + string(DARNER_VERSION) + "\r\n";
@nova77

nova77 Aug 16, 2012

Contributor

If DARNER_VERSION is just a basic C string, the proprocessor (iirc) should merge all that into a single string.

@erikfrey

erikfrey Aug 16, 2012

Contributor

ha, good point.

@nova77 nova77 commented on the diff Aug 16, 2012

src/queue/oqstream.cpp
{
+ if (queue_)
+ {
+ try
+ {
+ cancel();
+ }
+ catch (const std::exception& e) // we're in the dtor! we have to swallow everything
+ {
+ log::ERROR("oqstream::~oqstream(): %1%", e.what());
+ }
+ catch (...)
+ {
+ log::ERROR("oqstream::~oqstream(): unknown error");
@nova77

nova77 Aug 16, 2012

Contributor

Catching ... must be done with a lot of care.. As stated here:

Use catch (…) with caution; typically such a catch block is used to log errors and perform any special cleanup prior to stopping program execution. Do not allow a program to continue unless the catch block knows how to handle the specific exception that is caught.

@erikfrey

erikfrey Aug 16, 2012

Contributor

well, i'm trying to do the right thing and not throw in a dtor. is there a better way to handle this?

@nova77

nova77 Aug 16, 2012

Contributor

Yeah, that's a good point. I don't think there's really a better way to handle it after all.

Contributor

nova77 commented Aug 16, 2012

Looks great, just a few minor things.

@erikfrey erikfrey added a commit that referenced this pull request Aug 16, 2012

@erikfrey erikfrey Merge pull request #6 from wavii/clean_streams
Clean streams
9b7be61

@erikfrey erikfrey merged commit 9b7be61 into master Aug 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment