Opened 11 years ago

Last modified 11 years ago

#83 enhancement closed fixed (fixed)

Make adbapi obey min & max arguments

Reported by: davep Owned by: davep
Priority: normal Milestone:
Component: Keywords:
Cc: glyph, radix, spiv, davep, itamarst Branch:
Author: Launchpad Bug:

Description


Attachments (4)

twisted.diff (20.5 KB) - added by davep 11 years ago.
twisted.2.diff (19.5 KB) - added by davep 11 years ago.
twisted.3.diff (20.6 KB) - added by davep 11 years ago.
twisted.4.diff (24.4 KB) - added by davep 11 years ago.

Download all attachments as: .zip

Change History (19)

Changed 11 years ago by davep

comment:1 Changed 11 years ago by itamarst

Looks pretty good to me. Glyph?

comment:2 Changed 11 years ago by radix

you could probably do less work if you just create a new 
thread pool and pass on the min and max arguments - that 
way you let the threadpool handle pretty much everything 
for you.

Just a suggestion; I haven't figured out whether your 
solution is better yet.

comment:3 Changed 11 years ago by davep

Using a separate threadpool seems fairly straightforward, 
but I have one question: do I need to take the same care
that 'deferToThread' does in calling the callback or
errback from within the main reactor thread, instead
of the worker thread?

If that is the case, it might not be much simpler, though
it could still be preferable to the current patch.

comment:4 Changed 11 years ago by radix

Unless I'm missing something, all you should need to do is
something like this

from twisted.internet import threads, defer
def deferToThread(f, *args, **kwargs):
    d = defer.Deferred()
    myThreadPool.callInThread(threads._putResultInDeferred,
                              d, f, args, kwargs)
    return d

That's a very simple rape'n'paste of
twisted.internet.threads.deferToThread.

comment:5 Changed 11 years ago by davep

Yeah, you're right, I was thinking it would be more complicated.

Changed 11 years ago by davep

comment:6 Changed 11 years ago by davep

Here's a new patch that uses a threadpool instead.

I have come upon one problem: If you create a pool
in updateApplication, mktap never finishes since
there are threads in the pool. I'm not really sure
how to fix this. Add an option to ThreadPool to make
the threads daemon?

comment:7 Changed 11 years ago by radix

Hmm. I can't think of any elegant way to do this other than
making it a Service and implementing startService, but that
complicated the end-user API, I think. 

Hmmm -- what about doing a start-up event similar to how
you're doing the shutdown? Those get run only when
reactor.run() is called.

Also, for your "XXX start up min connections", I would
recommend making an interface for that in threadpool itself,
and calling it from adbapi.

Otherwise, from what I've seen, I like the patch.

Changed 11 years ago by davep

comment:8 Changed 11 years ago by davep

Here's a new version that solves the mktap problem
mentioned in msg158. I looked at using the 'startup' event,
but that won't work if the reactor is already running.

Is there a canonical way to test for that? It looks like
most of the reactors set reactor.running, but maybe not
all.

Anyway, this patch solves the problem using a callLater(0,
...) call.
This requires a slight modification to threadpool.py to
handle the case of work being added to the threadpool
before it is started.

comment:9 Changed 11 years ago by glyph

I think the problem may be that we need some replacement for
main.callWhenRunning.

Changed 11 years ago by davep

comment:10 Changed 11 years ago by davep

Here's a patch with a reactor implementation of 
callWhenRunning. It also modifies threadpool to
allow an init function to be called in each new
thread. The connection pool uses that to start up
the minimum connections.

comment:11 Changed 11 years ago by radix

This looks good to me (great work, davep). afaic, you can
commit whenever you want to.

comment:12 Changed 11 years ago by itamarst

Um, I just realized. Isn't reactor.callLater(0, foo) totally
equivalent to reactor.callWhenRunning(foo)? I don't think we
need to add anything to the reactor.

comment:13 Changed 11 years ago by davep

The only difference between the two is that if the reactor
is running, callWhenRunning will invoked the function
immediately.

I don't think that is a significant difference,though.

comment:14 Changed 11 years ago by itamarst

This patch should be commited. I guess callWhenRunning stays
for now, unless we can come up with better solution.

comment:15 Changed 11 years ago by itamarst

davep checked this in, woot

Note: See TracTickets for help on using tickets.