fishy commented on pull request #2500: URL: https://github.com/apache/thrift/pull/2500#issuecomment-1008092740
> only atomic.CompareAndSwapInt32 is atomic, first load and later store is not while technically that's correct, that doesn't matter here. the problem we are solving is that inside `TSocket.Close` and `TSSLSocket.Close`, we call underlying socket's close, and set the underlying socket to nil. The race condition is about two goroutines setting the socket to nil concurrently, not about the socket's Close being called concurrently, as that's already supported by the stdlib. so the fix here is that we use atomic int being set to non-zero to replace the old socket is nil check, and eliminate the race condition. so we can still get into the situation that two goroutines call `Close` concurrently, they both pass the `isValid` check, and get to the point of setting the atomic int to 1. this is still fine because two goroutines calling store is fine (the winner change it from 0 to 1 and the loser doesn't really make any changes), and then they will both call the underlying Close and that's still fine (the winner will close the socket and return nil error, the loser will get an already closed error). we can change it to compare and swap here, and return the already closed error here directly (like in the failed `isValid` check case), but the already closed error returned by the underlying Close function contains extra info about the connection itself, which is slightly better than the "naked" error we are returning here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
