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

Database Connection Timeout & Threading Shipping Calculation Error #2429

Closed
iloveitaly opened this Issue Jan 13, 2013 · 15 comments

Comments

Projects
None yet
5 participants
Member

iloveitaly commented Jan 13, 2013

The recently introduced threaded shipping calculation has the possibility of causing a ActiveRecord::ConnectionTimeoutError to be thrown.

While a thread is working on pulling shipping calculations from the UPS/Fedex/etc it grabs a unique database connection from the pool. I'm not 100% sure, but I'm guessing that the number of threads running in the background calculating shipping would be equal to number of users calculating shipping * number of shipping methods + 1 (one extra, because rails always holds on to one connection).

If more than one user is checking out at the same time it's probable that the wait time for the next available DB connection will exceed checkout_timeout (I believe the default is 5 seconds) and the end user will receive a error message.

An exception could also be raised if some of the shipping calculation endpoints are sluggish and take more than 5s to respond.

I'm not very familiar with threading in rails, but I don't see any way around this other than making the shipping calculation synchronous, or requiring users to adjust their database.yml connection pool settings (number of workers * number of shipping methods + 1). That doesn't seem like a very robust solution though (every time you change shipping options you'll have to update database.yml)

Maybe shipping calculation threading could be optional?

@iloveitaly iloveitaly referenced this issue in spree-contrib/spree_active_shipping Jan 13, 2013

Closed

Connection Time Out When Calculating Shipping #52

Member

radar commented Jan 13, 2013

How would you enable this to be synchronous or asynchronous and keep the code clean at the same time? If it's causing hassles, we should probably revert it completely.

Member

radar commented Jan 13, 2013

This is also similar to #2290.

Member

iloveitaly commented Jan 14, 2013

I'm not sure how making it optional could be done – to be honest, I didn't dig into the code that much.

I would be for making it synchronous – although it makes the user wait another second or two, the alternative (500 error page during the checkout process) is much worse.

Member

radar commented Jan 14, 2013

Ping @jsqu99 on this one. What do you think about switching it back to being synchronous? Have you ever had this problem with your setup?

Contributor

jsqu99 commented Jan 14, 2013

I haven't run into any problems but I understand the OPs scenario.

Spree survived for a long time w/out the threading, so it's fine to revert, and we can always revisit this in the next performance review?

Member

BDQ commented Jan 14, 2013

Just bump the pool value in the config/database.yml to compensate for this. I think the default is 5 and I've seen similar problems when testing threaded app servers. No need to revert this IMO.

At most we might need to limit the number of threads spawned (if its more than one), or just build an initializer check into Spree for a larger than default connection pool size.

Member

iloveitaly commented Jan 15, 2013

I like @BDQ's idea of alerting the user on application init (via Rails.logger.error). If I would of see that message when launching the app I would of increased the pool number and avoiding the problem completely.

Member

radar commented Jan 15, 2013

Do people even check their production logs? My concern with that method is
that it would be ignored.... but we could still try it anyway. We know what
the problem is, and so therefore we can direct people here when they run
across it. Bumping the pool size seems to be the right way of fixing this
problem, afaik.

Is there anything else we need to do on this issue?

On Tue, Jan 15, 2013 at 11:07 AM, Michael Bianco
notifications@github.comwrote:

I like @BDQ https://github.com/BDQ's idea of alerting the user on
application init (via Rails.logger.error). If I would of see that message
when launching the app I would of increased the pool number and avoiding
the problem completely.


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/issues/2429#issuecomment-12247115.

Member

iloveitaly commented Jan 20, 2013

I don't normally check my production logs, but if something is going wrong, I'll dive and in and see if there anything helpful. I think including a message to the error log would be helpful to people running into this issue.

Aside from the error, if no code changes are made, I think it should be mentioned on the shipping documentation page in one of the call out boxes.

Member

iloveitaly commented Jan 23, 2013

I think the threading here has something to do with spree/spree_active_shipping#53.

ActiveMerchant::Connection constructor clearly accepts one argument. However, the code triggering the error is acting like it isn't.

I never saw this error with the non-threaded version of spree shipping calculation. I don't really know why ActiveMerchant::Connection's constructor would be rewritten, but my hunch is that is has to do with the threading.

Does anybody have a good article on MRI ruby 1.9.x and threading? I need to learn a bit more here.

Member

iloveitaly commented Jan 28, 2013

After switching to the non-threaded version of this code the ArgumentError exception has disappeared. Maybe active_shipping isn't thread safe?

Member

radar commented Mar 3, 2013

What's the progress on this issue? Should we still fix it?

Member

iloveitaly commented Mar 3, 2013

I've monkeypatched the old non-threaded code onto my production application and haven't had any issues since.

I think we should switch back to the non-threaded code. This seems to at least this issue and #2290 and doesn't provide much benefit to the user.

fonemstr commented Mar 6, 2013

I've been experiencing this on my site. I'm in favor of switching back to non-threaded code.

Member

radar commented Mar 6, 2013

Great, thanks for the prompting, peeps.

I've got commits in to 1-3-stable and master to revert that threading now.

@radar radar closed this in 8432d7d Mar 11, 2013

radar added a commit that referenced this issue Mar 11, 2013

@iloveitaly iloveitaly referenced this issue in spree-contrib/spree_active_shipping Jan 21, 2013

Open

Wrong Number of Arguments for USPS Calculator #53

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