-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
only reconnect if we receive EXIT from the conn pid #27
base: master
Are you sure you want to change the base?
only reconnect if we receive EXIT from the conn pid #27
Conversation
c5803cf
to
4d6ebbf
Compare
[self(), Conn, Reason, NewDelay]), | ||
{noreply, State#state{conn = undefined, delay = NewDelay, timer = Tref}}; | ||
|
||
handle_info(_Other, State) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want that extra clause?
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 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! |
I didn't get a chance to write some proper example code to reproduce it, but essentially this is how the issue is triggered:
|
@aktungmak ok... that works. What kind of real world code was causing something like that? |
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 |
Would it make sense to do something like wrap the other library with a receive looking for its exit? |
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 would your refactoring still work if we include your patch, without the 'catchall' |
Yep, that will be just fine 👍 |
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. |
No description provided.