Improve logging: reduce verbosity for general messages and prevent cr…#3
Improve logging: reduce verbosity for general messages and prevent cr…#3
Conversation
…ash logs - Lower generla log level - Use shutdown error reasons to prevent crash logs - Better gun error handling - Better connection cleanup
sylane
force-pushed
the
sylane/better-logging
branch
from
a53bdae to
c5cf234
Compare
src/jarl_connection.erl
Outdated
| handle_common(info, {'DOWN', _, process, Pid, Reason}, StateName, | ||
| Data = #data{gun_pid = Pid}) -> | ||
| ?JARL_INFO("gun process of the connection to ~s crashed: ~p", | ||
| [Data#data.uri, Reason], | ||
| #{event => ws_gun_crash, uri => Data#data.uri, | ||
| reason => Reason, state => StateName}), | ||
| {stop, Reason, connection_closed(Data, closed)}; | ||
| ?JARL_DEBUG("Gun process for the connection to ~s terminated: ~w", | ||
| [Data#data.uri, Reason], | ||
| #{event => ws_exit, uri => Data#data.uri, | ||
| reason => Reason, state => StateName}), | ||
| SafeReason = case Reason of | ||
| normal -> normal; | ||
| {shutdown, R} -> {shutdown, R}; | ||
| R -> {shutdown, R} | ||
| end, | ||
| {stop, SafeReason, Data}; |
There was a problem hiding this comment.
With the code in terminate/3 and connection_close/2 I have the impression that we will try to stop gun a second time. In connection_close/2, gun:shutdown(GunPid) is called but if we receive the DOWN message that means that gun is already down.
Could this trigger a crash ?
There was a problem hiding this comment.
From the docs it looks like gun:shutdown tells gun to gracefully stop and prevents itself from re-attempting to connect.
There was a problem hiding this comment.
With the code in
terminate/3andconnection_close/2I have the impression that we will try to stop gun a second time. Inconnection_close/2,gun:shutdown(GunPid)is called but if we receive theDOWNmessage that means that gun is already down. Could this trigger a crash ?
@GwendalLaurent This is why I am not calling connection_close anymore when stopping. I am not sure I am understanding your concerns...
There was a problem hiding this comment.
I think the concearn is about calling gun:shutdown(GunPid) when you already received a DOWN from such Pid.
MAybe you could set the Pid to #data{gun_pid = undefined} to avoid calling gun:shutdown later
There was a problem hiding this comment.
you are calling connection_close in terminate
There was a problem hiding this comment.
Yes exactly, you receive a DOWN message indicating that gun is already down but then still call gun:shutdown(GunPid) during the terminate flow (in connection_close/2) . My concern is that the GunPid won't exist anymore and trigger a crash
There was a problem hiding this comment.
ok, I got it, nice catch. I don't think this is crashing, otherwise the tests would have failed, but I'll put back the call to connection_closed/2.
3 participants
…ash logs