asyncnet: don't try to close the socket again [backport] #15174
Conversation
alaviss
changed the title
dom96
left a comment
dom96
left a comment
There was a problem hiding this comment.
IMO an exception should be raised if you try to close an already closed socket. Maybe you can raise an exception instead of doing nothing?
lib/pure/asyncnet.nim
Outdated
| elif res != 1: | ||
| raiseSSLError() | ||
| socket.closed = true # TODO: Add extra debugging checks for this. | ||
| if not socket.closed: |
There was a problem hiding this comment.
Prefer:
if socket.closed: return
Style rather than wrapping everything in more and more nested ifs.
The closed flag isn't a good design by any means, but let's have this working first before I get rid of the flag and potentially create a non-backportable commit.
alaviss
force-pushed
the
asyncnet-double-close-fix
branch
from
dcf8946 to
fecaa03
Compare
|
|
it always calls |
|
Nah, |
|
That sounds dangerous, but alright, in any case I would still match the semantics in |
|
Currently I'm not doing that because I'm not invalidating the socket. I can certainly do so, but IMO it's better to do that with the follow up refactoring to get rid of the |
The closed flag isn't a good design by any means, but let's have this working first before I get rid of the flag and potentially create a non-backportable commit. (cherry picked from commit 957bf15)
The closed flag isn't a good design by any means, but let's have this working first before I get rid of the flag and potentially create a non-backportable commit. (cherry picked from commit 957bf15)
3 participants
The closed flag here doesn't seem to be a a good design to me, but until
I can understand why it was needed it seems unwise to just replace it
with the invalidation of the socket.
Fixes #9867 (comment)
/cc @dom96 as this
closedflag was added by him. This flag appear to have survived the refactoring in 1661062, where its sisternetmodule use the different approach of invalidating theSocketinstead of a flag.