pharo-nosql / mongotalk

A Pharo driver for MongoDB
MIT License
19 stars 13 forks source link

Added an ensure block in the MongoPool so it does not leak connection… #100

Closed noha closed 3 years ago

noha commented 3 years ago

…s in case of non-local returns that are used in the same driver.

noha commented 3 years ago

I need to read it more thoroughly. Maybe I have the wrong context. But:

Indeed I did not spend a lot of time thinking about it. To my current knowledge there are three possible things that could happend:

  1. a successful execution. In this case the current code is vulnerable to non local returns and the mongo driver uses exactly these. So the leakage of connections is sure. I did it at a high rate and exhausted my semaphore count within a short time frame
  2. a non-network error occurs. This is the uncovered variant. The current code will skip the returning to the pool although the error is not connection related. It will also skip a lot of functionality the stack above for any error that might occur
  3. a network error occurs. The on:do: block just resets the pool

For 1. the ensure block only matters for non-local returns. For 2. the ensure: block enables returning a connection for non connection related errors. But it means that there is an error in the voyage code and the sense of either way is questionable. Or there is a place where domain code gets executed and that should be avoided. For 3. the ensure: block will be executed first and returns the connection to the pool and then the on:do: block will reset it.

This is my opinion.

zecke commented 3 years ago

I need to read it more thoroughly. Maybe I have the wrong context. But:

  • I don't get the statement of @zecke that #on:do: is called before #ensure: If ensure: is the inner block it will come first

What I probably wanted to write is that we first return a connection to the pool and then reset the pool.

  1. a non-network error occurs. This is the uncovered variant. The current code will skip the returning to the pool although the error is not connection related. It will also skip a lot of functionality the stack above for any error that might occur

Oh. What I missed is a race condition. We might return a connection to the pool (and hand it out) that is somehow broken ("wire protocol error", didn't read the entire result from the socket and might get confused on the next iteration). I was focussed about the reset after return case that I missed that.