pgapp icon indicating copy to clipboard operation
pgapp copied to clipboard

only reconnect if we receive EXIT from the conn pid

Open aktungmak opened this issue 6 years ago • 10 comments

aktungmak avatar Dec 14 '18 09:12 aktungmak

If we don't have that clause, then the worker will crash any time it receives a message other than the 'EXIT' from epgsql_sock. When using pgapp:with_transaction this can occur since the transaction function is run by the worker process.

aktungmak avatar Dec 16 '18 21:12 aktungmak

@aktungmak is there a way to trigger that that always works, so I can test it out?

I do not like 'catch all' handlers, as they may let problems sneak in.

Thanks!

davidw avatar Dec 17 '18 20:12 davidw

I didn't get a chance to write some proper example code to reproduce it, but essentially this is how the issue is triggered:

pgapp:with_transaction(fun() -> self() ! {'EXIT', self(), normal} end)

aktungmak avatar Dec 18 '18 20:12 aktungmak

@aktungmak ok... that works. What kind of real world code was causing something like that?

davidw avatar Dec 18 '18 20:12 davidw

The code was using a library that was spawn_linking other processes which then finished normally, and since the pgapp_worker process is trapping exits it was receiving {'EXIT', Pid, normal} messages. I wrote a bit more detail on this in issue #26.

aktungmak avatar Dec 18 '18 21:12 aktungmak

Would it make sense to do something like wrap the other library with a receive looking for its exit?

davidw avatar Jan 04 '19 00:01 davidw

We have refactored the code that was using the other library to perform the work outside the transaction now, which avoid the issue in this particular case.

aktungmak avatar Jan 07 '19 21:01 aktungmak

@aktungmak would your refactoring still work if we include your patch, without the 'catchall' handle_info ?

davidw avatar Jan 07 '19 21:01 davidw

Yep, that will be just fine 👍

aktungmak avatar Jan 07 '19 21:01 aktungmak

The only problem will be as I mentioned above, that the worker will crash if it receives any other messages inside the transaction function (eg if a spawn_linked process finishes normally).

We are no longer doing that so its OK by me but it is good to know in future that the transaction functions should avoid doing that.

aktungmak avatar Jan 07 '19 21:01 aktungmak