Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() ...................................................................... Patch Set 2: > Patch Set 2: > > > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > > > warning? > > > > > > The prior version of the method would take a reference to > > > > > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > > > warning? > > > > > > Thank you for looking at this. > > > > > > I quickly looked at the code around when putting together this > > > patch and I was under impression that 'timeout' was passed by > > > reference into the callback functor which on its turn was passed > > > into LookupTabletByKey(), so I suspected a functor being run with > > > de-allocated MonoTime. > > > > > > After taking another look, I found turns out the 'timeout' is > > used > > > to compute 'deadline' using 'timeout' right before doing anything > > > else, and then 'deadline' is actually passed around by _value_ in > > > CoordinateTransactionAsync(). > > > > > > So yes, it seems this might be a dud fix, but it should not hurt > > in > > > any other way, and we pass MonoDelta by value in many more other > > > cases anyway, so you consider this just as a 'style' fix :) > > > > > ... so you consider this just as a 'style' fix :) > > > > I meant "... so you can consider this to be just a 'style" fix :)" > > After another look, it seems the issue here is about passing > MonoDelta::FromNanoseconds(std::numeric_limits<int64_t>::max()) as a value > for 'timeout' parameter when calling > TxnSystemClient::CoordinateTransactionAsync() Ah indeed, I assumed this was a case of the MonoDelta being used in an async function but being destructed before actually being used. Good call-out Bankim. -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 23:22:53 +0000 Gerrit-HasComments: No
