not allowing QueryCache Rack to fail on database exception (#2142) #2227

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@skippy

skippy commented Jul 23, 2011

I've been testing this (see sample app: https://github.com/skippy/rails31QueryCache) and I actually think this is a bigger issue than raised in #2142. The query cache is triggered so early in the rack stack that if something goes wrong with the database (such as a misconfiguration or it goes down) it becomes a bit tricky to capture that error... you definitely can't do it from the controller level (it looks like hoptoad works as it monkey patches low enough to grab the error).

I think @pritchie has a valid point so here is my attempt to address it. if it fails in the QueryCache rack layer, let it continue through and if it fails somewhere else (most likely) then so be it; it will throw the same error but higher up on the stack where it can be handled by application-specific logic.

Please comment.

thanks!

@guilleiguaran

This comment has been minimized.

Show comment Hide comment
@guilleiguaran

guilleiguaran Jul 24, 2011

Owner

@skippy can you set a more descriptive message to the commit? :D

Owner

guilleiguaran commented Jul 24, 2011

@skippy can you set a more descriptive message to the commit? :D

@skippy

This comment has been minimized.

Show comment Hide comment
@skippy

skippy Jul 24, 2011

haha... yes

skippy commented Jul 24, 2011

haha... yes

@pritchie

This comment has been minimized.

Show comment Hide comment
@pritchie

pritchie Jul 25, 2011

@skippy that works, but will the query cache now be off for the life of the Rails process?

Feels like we should be able to set the query cache flag even if the database isn't currently available.

@skippy that works, but will the query cache now be off for the life of the Rails process?

Feels like we should be able to set the query cache flag even if the database isn't currently available.

@pritchie

This comment has been minimized.

Show comment Hide comment
@pritchie

pritchie Jul 25, 2011

I hate rescue Exception, can cause all kinds of unexpected behavior (rescued syntax errors etc...). How about rescuing StandardError instead?

I hate rescue Exception, can cause all kinds of unexpected behavior (rescued syntax errors etc...). How about rescuing StandardError instead?

@skippy

This comment has been minimized.

Show comment Hide comment
@skippy

skippy Jul 25, 2011

@pritchie, addressing the 3 points in order:

  • the query cache will not be turned on IF the original connection fails; otherwise it will be there.
  • The querycache was baked into the connection, it seems, to just keep things clean, and now to pull it out seems messy. I made the change that I did to try and keep it small and focused rather than a (much) larger patch to decouple it completely from the connection. But I also think that is the right approach rather than having dependencies on the middleware (or some helper class). The other way to look at this is to not have ActiveRecord::Base.connection actually trigger a db call until used, but again, I think that is a thornier change outside of the scope
  • I looked at that, but the reason I kept it general is that the exceptions that are thrown are direct from the driver and are quite varied in type. I'm not sure you can make the expectation that they all stem from StandardError

skippy commented Jul 25, 2011

@pritchie, addressing the 3 points in order:

  • the query cache will not be turned on IF the original connection fails; otherwise it will be there.
  • The querycache was baked into the connection, it seems, to just keep things clean, and now to pull it out seems messy. I made the change that I did to try and keep it small and focused rather than a (much) larger patch to decouple it completely from the connection. But I also think that is the right approach rather than having dependencies on the middleware (or some helper class). The other way to look at this is to not have ActiveRecord::Base.connection actually trigger a db call until used, but again, I think that is a thornier change outside of the scope
  • I looked at that, but the reason I kept it general is that the exceptions that are thrown are direct from the driver and are quite varied in type. I'm not sure you can make the expectation that they all stem from StandardError
@pritchie

This comment has been minimized.

Show comment Hide comment
@pritchie

pritchie Jul 26, 2011

I think it's reasonable to expect driver errors to inherit from StandardError. See:

http://rubydoc.info/gems/mysql2/0.3.6/Mysql2/Error
http://rubydoc.info/gems/pg/0.11.0/PGError

I think it's reasonable to expect driver errors to inherit from StandardError. See:

http://rubydoc.info/gems/mysql2/0.3.6/Mysql2/Error
http://rubydoc.info/gems/pg/0.11.0/PGError

@pritchie

This comment has been minimized.

Show comment Hide comment
@pritchie

pritchie Jul 26, 2011

Not turning on the query cache if the original connection feels like a bug. Perhaps a smaller bug than failing to start up at all, but a bug all the same.

Lazy load feels like the right option, but from looking at /connection_adapters/abstract/connection_pool.rb that's going to be adapter specific... given that some form of middleware or helper class might be cleaner.

Not turning on the query cache if the original connection feels like a bug. Perhaps a smaller bug than failing to start up at all, but a bug all the same.

Lazy load feels like the right option, but from looking at /connection_adapters/abstract/connection_pool.rb that's going to be adapter specific... given that some form of middleware or helper class might be cleaner.

@skippy

This comment has been minimized.

Show comment Hide comment
@skippy

skippy Jul 27, 2011

hey @pritchie,
all your points sound valid, so I would say go for it and submit a patch with your thoughts in code.

skippy commented Jul 27, 2011

hey @pritchie,
all your points sound valid, so I would say go for it and submit a patch with your thoughts in code.

@arunagw

This comment has been minimized.

Show comment Hide comment
@arunagw

arunagw Dec 19, 2011

Member

What's happening to this PR guys??

Member

arunagw commented Dec 19, 2011

What's happening to this PR guys??

@skippy

This comment has been minimized.

Show comment Hide comment
@skippy

skippy Dec 19, 2011

@pritchie was correct in that it was not the cleanest; I was trying to minimize changes, but to do it 'properly' should probably convert some of the QueryCache class into using thread-local variables wrapped in class-scoped methods.

skippy commented Dec 19, 2011

@pritchie was correct in that it was not the cleanest; I was trying to minimize changes, but to do it 'properly' should probably convert some of the QueryCache class into using thread-local variables wrapped in class-scoped methods.

@arunagw

This comment has been minimized.

Show comment Hide comment
@arunagw

arunagw Dec 19, 2011

Member

Can we close this PR. And you can submit the new one. What you say?

Member

arunagw commented Dec 19, 2011

Can we close this PR. And you can submit the new one. What you say?

@skippy

This comment has been minimized.

Show comment Hide comment
@skippy

skippy Dec 19, 2011

you bet, but I would look to @pritchie to submit what he thinks is appropriate; I picked this up during the rails bug squash and I don't see time coming up to rework this... sorry!

skippy commented Dec 19, 2011

you bet, but I would look to @pritchie to submit what he thinks is appropriate; I picked this up during the rails bug squash and I don't see time coming up to rework this... sorry!

@skippy skippy closed this Dec 19, 2011

@arunagw

This comment has been minimized.

Show comment Hide comment
@arunagw

arunagw Dec 19, 2011

Member

No Probs @skippy . Thanks for your all hard work :-)

Member

arunagw commented Dec 19, 2011

No Probs @skippy . Thanks for your all hard work :-)

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