Conversation
manandre
force-pushed
the
clear-pool-async
branch
from
a24a9ad to
4a0e7ae
Compare
manandre
force-pushed
the
clear-pool-async
branch
3 times, most recently
from
c88aa35 to
858e407
Compare
vonzshik
left a comment
vonzshik
left a comment
There was a problem hiding this comment.
Yeah, sorry, was busy with other stuff. Probably a good idea for me to take a look at all pr's we have...
| // very unlikely to block (plus locking would need to be worked out) | ||
| internal void Close() | ||
| { | ||
| lock (this) |
There was a problem hiding this comment.
This particular lock is actually very important. It helps at least with:
- Concurrent close
- Concurrent keepalive
- Concurrent break
I'm not sure whether changing everywhere to use SemaphoreSlim is a good idea, would have to think about that.
There was a problem hiding this comment.
Yeah, still no idea what to do here. @roji maybe you have something on your mind?
| try | ||
| { | ||
| _semaphore.Release(); | ||
| _semaphore.Dispose(); |
There was a problem hiding this comment.
It seems like if we're not closing the connector (but, for example, breaking it while opening) the semaphore is not going to get disposed.
There was a problem hiding this comment.
We must guarantee that the Dispose/Close method is correctly called once a connector is not usable/used anymore.
Should we check everywhere that this statement is correctly enforced?
| // very unlikely to block (plus locking would need to be worked out) | ||
| internal void Close() | ||
| { | ||
| lock (this) |
There was a problem hiding this comment.
Yeah, still no idea what to do here. @roji maybe you have something on your mind?
manandre
force-pushed
the
clear-pool-async
branch
from
5b5d86e to
948022b
Compare
manandre
force-pushed
the
clear-pool-async
branch
from
948022b to
aa956b9
Compare
|
@manandre just to confirm, you're waiting on our review for this right? If not, can you please rebase and we'll do our best to review and merge ASAP? Not sure this will happen for 8.0 at this point, but we can always try. |
manandre
force-pushed
the
clear-pool-async
branch
from
aa956b9 to
4338dbd
Compare
|
@roji I have just rebased. However, I am still not fully confident around the lock replacement in the |
|
Same here, let's aim to get this into a patch for 8.0. Thanks @manandre |
4 participants
Fixes #4499