-
Notifications
You must be signed in to change notification settings - Fork 86
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
Memory leak in networking code #1902
Conversation
d336690
to
b9a62de
Compare
b9a62de
to
b3ccb1f
Compare
Requires: IntersectMBO/cardano-base#89 |
1c3159a
to
d59b2a7
Compare
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.
Looks good.
One comment below. Plus we can actually avoid changing cardano-base
.
@@ -72,17 +73,17 @@ socketAsMuxBearer sduTimeout_m tracer sd = | |||
Nothing -> do | |||
traceWith tracer $ Mx.MuxTraceSDUReadTimeoutException | |||
throwM $ Mx.MuxError Mx.MuxSDUReadTimeout "Mux SDU Timeout" callStack | |||
Just r -> return r | |||
Just !r -> return r |
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.
This one is a bit odd. It's a bang pattern on a pair right? It's the result of recvRem
so we should add strictness there instead.
Do it on the two components of the result pair (header {Mx.msBlob = blob}, ts)
.
!ts <- getMonotonicTime
and
let !header' = header {Mx.msBlob = blob}
return (header', ts)
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.
Pushed a patch to implement this suggestion.
Also remove unneccessary 'MuxSDUHeader' type from Mux.Codec module.
Simplify the NFData Expiry instance, since NF=WHNF for this type. Eliminate the now-unused NFData SlotNo instance.
d59b2a7
to
bf45bd7
Compare
Bang things nearer their construction site, and don't bang pairs.
I've dropped the changes that needed this and did it in a way that didn't need them. |
bors merge |
No description provided.