Return a request object?

[myndzi]

While addressing #14, it occurred to me that it might be useful to return a request object from pool.acquire(); this object could emit events for things such as "was queued to receive a resource but the resource failed the ping test, so got requeued", and provide an interface for manually aborting a request.

I started coding on it and then realized I wasn't sure if there was really a case for manually aborting a request. I think I'm going to limbo this idea with the others, in the interests of keeping the code as simple as possible, until I see some use cases for implementing. Please chime in if you can think of something or have a need for this feature!

[vellotis]

Actually I already made a fix for that. I haven't looked the tests yet. But what do you think of it?

vellotis@55efa39a7d88a6016f1af485ff1f54f5e383e49d

The reason I started working on it is I need it for knex/bookshelf as I need to get some info when the DB is down or etc. This is the only reasonable way in my mind.

The second thing I am thinking on is that maybe there should be a way to provide a method by opts that knows how to convert resource error into library/application suitable one. I would be really nice to use it for promise or 'instanceof' operator.

[vellotis]

Actually I like your idea of separating ResourceRequest the way you do.

[vellotis]

And the way I see the error conversion for my version of the suggested implementation.

vellotis@72cc9306261b9e72b0728145fe744baa4d03bbb2

[myndzi]

The branch isn't complete but I pushed it to this repository (https://github.com/myndzi/pool2/tree/request-refactor).

The separation of request from resource management was intentional in the design of pool2; I wanted to have as little of the actual resource management involved with the consumer API as possible. The job of the pool is to make it so you don't have to worry about the details of resource management; building in functionality to allow you to worry about resource management and/or meddle with it is counterproductive. That's why I'm uncertain about this feature.

On the other hand, information definitely needs to be provided that will enable a consumer to perform some sort of health monitoring if they want to. I think it's a no-brainer to add a few more events, even if they won't necessarily get used very often, similar to how node's net and http servers have various events related to socket and request lifecycles, some of which you can safely ignore. I think this will handle your "keeping an eye on the health of the system" use case. Adding some sugar keys to the constructor options will even make it easy to expose this functionality to consumers of libraries that use pool2, such as knex, so long as they pass along upstream any options you provide (I believe knex does this).

The "error conversion" idea doesn't belong in this module at all; that belongs in your application code. I know it's a pain that you can't just shoehorn a one size fits all callback in here, but really, your application code is the only place that will have the context to add meaning to the error messages. I'm not ruling it out just yet, but you're going to have to make a pretty good case for it that involves explaining why it belongs here and not in your application code.

[myndzi]

P.S. - the request timeout option was mentioned in #7, I'll take your comments here to mean that you would like that as a feature?

[vellotis]

I agree with you. Error converting is not a part of the pool. I found a place in knex where it should be done. My request can be forgotten.

Pardon me for commentig request timeout problem in the current discussion. Have ran throught bookshelf->knex->pool2 issues and had a little mess up to choose the correct issue to comment.

But I think that I have some recommendation for ResourceRequest. As I see that there should be a way to timeout requests, it would be nice if the acquire method could accept timeout argument explicitly. And the ResouceRequest should have timer to timeout by itself by setting a boolean property. But as pool2 uses deque for requests the whole stack should be looped (as in vellotis@55efa39) to respond to the resource acquiring request.

[myndzi]

If you see my patch, I attacked this differently. I just flagged the resource request as fulfilled and left it in the queue. The allocation code simply drops fulfilled requests when it encounters them, avoiding the 'regenerate the whole thing' mess. This would affect the stats some, so it's not ideal, but if I had to arbitrarily splice items out of the queue I'd probably just use a simple array in the end. I don't actually think the double-ended-queue module is much of a benefit given the purpose of this library.

I agree that being able to specify a timeout at allocation time is useful, though it may affect the API to implement this. It would also require changes in knex to be able to take advantage of, since the pool isn't directly used by a person creating a knex query.

[vellotis]

I examined your patch more deeply. If this ResourceRequest had a timeout timer then I'd have to say that I like your solution better than mine. I wouldn't watch past the deque module. Speed is always a benefit.

At the moment knex doesn't have the functionality to provide a request timeout. But it would be a nice feature. But I still see a need to apply a default request timeout through configuration for ResourceRequest.

[myndzi]

Yes, that's where I'd implement it (in the ResourceRequest method).

The thing about Deque being "faster" is that a resource pool is explicitly designed to essentially keep slow resources around so you don't have to reopen them. The nature of a pool is already "slow", and if you're seeing the kind of utilization where Array prototype methods are your bottleneck, I commend you on the insanity that is your code ;) I primarily used it because it fit the use case I was putting it to, and when that is the case, I like to use it. That's about all.

[vellotis]

I would gladly continue this topic and touch as well #7 again as I am very interested to solve this some how.

It is very conceptual which way it should be implemented. Isn't the main question about where should be the resource healthyness insured? By the pool or by the module usingthe pool?

As the pool acquires connection resources and holds them, my opinion is that the pool should also be responsible of the healthyness of them. The only benefit for returning the resource request, which you already mentioned, is that it could be cancellable. The current error for the resource request has to be sent to the requester either way.

But still I see a big benefit for separating ResourceRequest as an event emittable module. I started to improve your initial implementation, but I ended up seeing that it needs some simplification.

What are your thoughts?

[vellotis]

As I really need this feature I ended up in improving your request-refactor branch. Now I just would like to hear your thoughts to refactor it the way it hopefully would please you too.

[myndzi]

Hey. Just letting you know I'm not ignoring you, but looking for time to go over and finish up this work. I'm more careful with this project than I might otherwise be about new code because Knex depends on it, so it can affect a lot of people.

[myndzi]

I think where things got out of hand is you tried to translate the request resolution into events, which is not how it should go. Request resolution translates directly to the callback supplied to acquire (pool.acquire(callback)) -- if a request is aborted or times out, that callback receives an error. Once you did that you had to start messing with a bunch of other stuff to account for it, or at least that's how my skim of it reads. The request process should not require changes to any of the pool lifecycle stuff.

I've updated the refactor branch and written tests against it. I think I'm okay to merge it (existing behavior is maintained; existing tests pass with the new code), but I'd like to verify it covers what you needed.

There is now a requestTimeout option passed to pool to specify a timeout. pool.acquire returns a request object that exports methods:

• setTimeout: sets the request's timeout. This is inclusive of elapsed time, so if you set the timeout to 10 seconds after the request has existed for 5 seconds, the timeout will happen 5 more seconds from the time you called setTimeout. If you set the timeout to a value less than the elapsed time, the request will time out immediately.
• clearTimeout: clears a request's timeout
• resolve: assign a resource to this request. Should only be used internally by pool2
• reject: cause this request to fail. Should only be used internally by pool2
• abort: abort the request with an optional message. This should be used instead of reject

The pool itself will emit two new events:

• request: emitted with the request object when it is created
• requeue: emitted when a request object is put back in the queue. This will happen if, for example, a request and corresponding resource were allocated but the ping failed.

The resource request will also emit errors for either rejections or redundant fulfillments (resolve or reject were called when the request had already been fulfilled). This is primarily informational, and these events should not be used in your code. They are converted to 'warn' events by pool2 itself so as not to cause crashes. In the case where a request has been fulfilled already, and something calls reject (or abort, or a timeout expires -- which shouldn't happen), two events will be emitted: the original error and a second error (redundant fulfill).

[myndzi]

Originally I had thought the request object was going to be emitting events for requeues, but when I was coding it, it didn't make any sense at all. I believe I could reduce the whole resource request thing to being simply a promise or promise-derived function (I don't see much use for dynamically adjusting the timeout, so it could really come down to something like Promise.race([Promise.delay(N), requestPromise]).nodeify(callback)). The only thing that would be given up is the notifications of redundant resolutions, which aren't a case that should really crop up anyway.

[vellotis]

Hey. My appologies for not being able to respond so long. I agree that things got out of hand in my "try". Didn't like it much by myself also.

I must say that I am happy with your solution. :+1: And I would be glad if you could merge it to master.

I dropped the "immediate error responding" functionality request after I made a little research on the internet on the other pool libraries. None of them return an error before a request timeout is reached.

[myndzi]

No worries! I've been doing a lot myself. I'm glad it solves your problem; I just didn't want to merge in new changes like this and take the "change risk" if it wasn't worth it. Thanks for getting back.