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

Changed RedisSubscriber wait with TaskCondition vs Timers #815

Merged
merged 3 commits into from Oct 13, 2014

Conversation

Projects
None yet
3 participants
@etcimon
Contributor

etcimon commented Sep 10, 2014

This update provides stability and performance improvements to the RedisSubscriber:

  • Uses TaskCondition notifications rather than rearming timers throughout the class.
  • Permits queuing actions in case multiple tasks are communicating, the old Timer/null check method had undefined behavior.
  • Fixes an error with the ConnectionPool, where connections could be re-used after they were closed. An assertion on the new event driver required this fix.
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 11, 2014

Contributor

Ok, this is good to go

Contributor

etcimon commented Sep 11, 2014

Ok, this is good to go

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 12, 2014

Member

Fixes an error with the ConnectionPool, where connections could be re-used after they were closed.

This is not an error. ConnectionPool's design is based on the assumption that connections can be re-established. For this reason _request_reply checks if the connection is still alive before each request.

Member

s-ludwig commented Sep 12, 2014

Fixes an error with the ConnectionPool, where connections could be re-used after they were closed.

This is not an error. ConnectionPool's design is based on the assumption that connections can be re-established. For this reason _request_reply checks if the connection is still alive before each request.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 12, 2014

Contributor

Ok, I'd have to fix the driver about this assumption. Maybe a specific unit tests would help, but would it be acceptable as a feature addition to remove connections though?

Contributor

etcimon commented Sep 12, 2014

Ok, I'd have to fix the driver about this assumption. Maybe a specific unit tests would help, but would it be acceptable as a feature addition to remove connections though?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 12, 2014

Member

That may make sense in some situations (although it's probably more important to add support for limiting the number of connections), but it should be separately dealt with (there should be a clear vision of the use cases and implications).

Member

s-ludwig commented Sep 12, 2014

That may make sense in some situations (although it's probably more important to add support for limiting the number of connections), but it should be separately dealt with (there should be a clear vision of the use cases and implications).

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 12, 2014

Contributor

When you say connections can be re-established, you mean that a TCPConnection that was closed can be used to send data again without warning? Wouldn't that be prone to errors regarding different TCP options and more complex to dispose and resume at a lower level? I would have assumed using a new object would be safer

Contributor

etcimon commented Sep 12, 2014

When you say connections can be re-established, you mean that a TCPConnection that was closed can be used to send data again without warning? Wouldn't that be prone to errors regarding different TCP options and more complex to dispose and resume at a lower level? I would have assumed using a new object would be safer

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 12, 2014

Contributor

Oh I see what you mean, now I realiaze I had an error in the TCPConnection.close() function that made TCPConnection.connected true for a closed connection. I'll revert the ConnectionPool changes and make the necessary changes to the driver!

Contributor

etcimon commented Sep 12, 2014

Oh I see what you mean, now I realiaze I had an error in the TCPConnection.close() function that made TCPConnection.connected true for a closed connection. I'll revert the ConnectionPool changes and make the necessary changes to the driver!

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 12, 2014

Contributor

However, the pull is still necessary because the handling for unsubscribe/subscribe responses takes priority over publish messages, and it doesn't quite support multiplexing either, so this implementation is currently completely unsafe (old me from then was stupid ;).

Contributor

etcimon commented Sep 12, 2014

However, the pull is still necessary because the handling for unsubscribe/subscribe responses takes priority over publish messages, and it doesn't quite support multiplexing either, so this implementation is currently completely unsafe (old me from then was stupid ;).

Show outdated Hide outdated source/vibe/db/redis/redis.d Outdated
Show outdated Hide outdated source/vibe/db/redis/redis.d Outdated

@s-ludwig s-ludwig added needs input and removed needs review labels Oct 7, 2014

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 12, 2014

Contributor

This is ready for a review

Contributor

etcimon commented Oct 12, 2014

This is ready for a review

Show outdated Hide outdated examples/redis-pubsub/source/app.d Outdated
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 12, 2014

Member

Just a quick observation: Some changes in json.d ended up in this PR, they should be removed. Similarly, there are a lot of whitespace changes in redis.d, which should be removed (especially since the first thing my editor would do would be to remove the trailing WS again).

I'll try to review the actual changes later today.

Member

s-ludwig commented Oct 12, 2014

Just a quick observation: Some changes in json.d ended up in this PR, they should be removed. Similarly, there are a lot of whitespace changes in redis.d, which should be removed (especially since the first thing my editor would do would be to remove the trailing WS again).

I'll try to review the actual changes later today.

Show outdated Hide outdated source/vibe/db/redis/redis.d Outdated
@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 13, 2014

Contributor

looks good to me, what do you think @s-ludwig ?

Contributor

Extrawurst commented Oct 13, 2014

looks good to me, what do you think @s-ludwig ?

// This is a simple parser/handler for subscribe/unsubscribe/publish
// commands sent by redis. The PubSub client protocol is simple enough
void pubsub_handler() {

This comment has been minimized.

@s-ludwig

s-ludwig Oct 13, 2014

Member

Looks like there should be a scope (failure) m_lockedConn.conn.disconnect(); here, so that any parsing exception doesn't leave the connection in a bad state.

@s-ludwig

s-ludwig Oct 13, 2014

Member

Looks like there should be a scope (failure) m_lockedConn.conn.disconnect(); here, so that any parsing exception doesn't leave the connection in a bad state.

This comment has been minimized.

@etcimon

etcimon Oct 13, 2014

Contributor

This should be covered with the scope(exit) on line 927, since the pubsub_handler() is only called through the while loop under there. The only points of failure now that I think of it would be runTask and vibe.concurrency.send at line 919..., I'm not sure if these functions can throw though?

@etcimon

etcimon Oct 13, 2014

Contributor

This should be covered with the scope(exit) on line 927, since the pubsub_handler() is only called through the while loop under there. The only points of failure now that I think of it would be runTask and vibe.concurrency.send at line 919..., I'm not sure if these functions can throw though?

This comment has been minimized.

@s-ludwig

s-ludwig Oct 13, 2014

Member

Okay, I see. Maybe it makes sense to convert the nested functions into private member functions instead, so that the function gets a bit more manageable. But that doesn't need to be done for this PR.

@s-ludwig

s-ludwig Oct 13, 2014

Member

Okay, I see. Maybe it makes sense to convert the nested functions into private member functions instead, so that the function gets a bit more manageable. But that doesn't need to be done for this PR.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 13, 2014

Member

I have to admit that my review hasn't been as thorough as I had hoped, but apart from the scope thing above (btw, m_lockedConn.destroy(); should probably also be in the scope statement), it looks good to merge.

Member

s-ludwig commented Oct 13, 2014

I have to admit that my review hasn't been as thorough as I had hoped, but apart from the scope thing above (btw, m_lockedConn.destroy(); should probably also be in the scope statement), it looks good to merge.

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 13, 2014

Contributor

well i have to admit i am not familiar enough with the whole subscriber design in general, but the current code in trunk is simply broken. something has to be done.

Contributor

Extrawurst commented Oct 13, 2014

well i have to admit i am not familiar enough with the whole subscriber design in general, but the current code in trunk is simply broken. something has to be done.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 13, 2014

Member

No doubt about that!

Member

s-ludwig commented Oct 13, 2014

No doubt about that!

s-ludwig added a commit that referenced this pull request Oct 13, 2014

Merge pull request #815 from etcimon/redis-conditions
Changed RedisSubscriber wait with TaskCondition vs Timers

@s-ludwig s-ludwig merged commit 8b98831 into vibe-d:master Oct 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment