-
Notifications
You must be signed in to change notification settings - Fork 1
Fix OSS build #1
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
base: master
Are you sure you want to change the base?
Conversation
34b6dda
to
cb5455f
Compare
Hi @mszabo-wikia, I have been following the developments of your branches closely. I want to thank you for fixing the bit rot on hhvm@next. I have given you a shout out on my GH. I have built hhvm from source, with your branch as a base, for my OSS CI and for use deployed projects. I have a change that might be good to include in your upstream, added support for
These commit links might become invalid when I rebase on a later change of yours and force push, so hereby also a textual description of the changes made. The first two remove I hereby give you, and anymore else reading this, permission to use these diffs, in whole or in part, with or without attribution, for any purpose. My employer is aware I contribute to open source and has carved out an explicit permission for me to contribute in my personal capacity. |
@hershel-theodore-layton Thank you, I appreciate it. There's indeed no need to exclude these extensions now that the squangle maintainer has fixed the OSS build issues that were blocking it a few months back :) Thanks for your patches. Strangely the stray test file didn't cause any build errors locally for me, but you're correct that it shouldn't be part of a production build either way. I opted to remove it from the source list so it should no longer cause issues. As for the failing CI here... my sources say that GitHub Actions was disabled over at the main repo due to my overly enthusiastic rebasing causing a budget overrun, which certainly was not part of the plan. Sadly the default actions runners don't have enough free disk space to accommodate an HHVM build by default, but there should be ways to fix this by removing cruft from the default runner (which however also requires containerizing only the package build step instead of the entire job, else any attempt at cleanup won't be able to remove anything from the runner proper). It'll take some fiddling, but should be doable. LMK if you have any questions. |
7055206
to
d575db5
Compare
Summary: The `MicroLock` tests fail under libc++ with asan-abort `stack-buffer-overflow`. Example: ``` SCARINESS: 32 (4-byte-read-stack-buffer-overflow) #0 unsigned int std::__2::__cxx_atomic_load[abi:ue170004]<unsigned int>(std::__2::__cxx_atomic_base_impl<unsigned int> const*, std::__2::memory_order) libcxx/include/c++/v1/__atomic/cxx_atomic_impl.h:375 #1 std::__2::__atomic_base<unsigned int, false>::load[abi:ue170004](std::__2::memory_order) const libcxx/include/c++/v1/__atomic/atomic_base.h:60 facebook#2 folly::MicroLockBase<1000u, 0u>::try_lock() folly/MicroLock.h:319 facebook#3 SmallLocks_MicroLockTryLock_Test::TestBody() folly/synchronization/test/SmallLocksTest.cpp:314 ``` The trouble is accessing the lock state as a 4-byte word, where the asan stack has allocated only 1 byte for the lock object - asan tracks this and aborts. It is UB for `MicroLock` to perform atomic stores or atomic read-modify-write operations on the entire 4-byte aligned word containing the `MicroLock` which may be concurrent with other accesses to other bytes in the word. The 4-byte aligned word accesses are necessary in order to support using the `MicroLock` itself as a futex, on Linux platforms. The trouble is that Linux does not provide futex operations on all sizes of integer, only on 32-bit (4-byte) integers. If Linux were to provide futex operations on all sizes of integer, we would immediately switch to 8-bit (1-byte) integers here. This diff fixes the UB and gets the tests passing under libc++ by switching to the `folly::ParkingLot`, as wrapped by `folly::atomic_notify_one` and `folly::atomic_wait` over `std::atomic<uint8_t>`, which indirects the 32-bit (4-byte) futex behind a sharded hashtable. Alternatives are possible for getting the tests passing, although these alternatives would not fix the UB. * We can disable sanitizers on even more functions, as well as switch to atomic builtins since the members of `std::atomic` are not marked as disabling the sanitizers. * We can workaround just in the unit-tests by padding the on-stack `MicroLock` instances. Reviewed By: praihan, ot, aary Differential Revision: D76612658 fbshipit-source-id: 75e9f03e63c85c30c6201b9bce5cd303c9a2b530
d575db5
to
539ffa8
Compare
Summary: After recent Python 3.12 rollout we started seeing crashes, e.g. T223049180. Crash stack from core dumps points to tstate being null when `_PyErr_GetRaisedException` is called ``` * thread #1, name = 'server#native-m', stop reason = SIGSEGV: address not mapped to object (fault address=0x60) * frame #0: 0x00007f94039e0197 libpython3.12.so.1.0`_PyFrame_MakeAndSetFrameObject [inlined] _PyErr_GetRaisedException(tstate=0x0000000000000000) at errors.c:490 frame #1: 0x00007f94039e0197 libpython3.12.so.1.0`_PyFrame_MakeAndSetFrameObject [inlined] PyErr_GetRaisedException at errors.c:499 frame facebook#2: 0x00007f94039e0184 libpython3.12.so.1.0`_PyFrame_MakeAndSetFrameObject(frame=0x00007f940977f340) at frame.c:32 frame facebook#3: 0x00007f9403b9d3de libpython3.12.so.1.0`PyThreadState_GetFrame [inlined] _PyFrame_GetFrameObject(frame=<unavailable>) (.__uniq.305758459065587366612134685926705492046) at pycore_frame.h:213 frame facebook#4: 0x00007f9403b9d3d0 libpython3.12.so.1.0`PyThreadState_GetFrame(tstate=<unavailable>) at pystate.c:1754 ``` The problem happens when this code is called (https://fburl.com/26dhwv4v) ``` PyObject * PyErr_GetRaisedException(void) { PyThreadState *tstate = _PyThreadState_GET(); return _PyErr_GetRaisedException(tstate); } ``` Where `_PyThreadState_GET` is implemented as follows https://fburl.com/92hrrluf ``` /* Get the current Python thread state. This function is unsafe: it does not check for error and it can return NULL. The caller must hold the GIL See also PyThreadState_Get() and _PyThreadState_UncheckedGet(). */ static inline PyThreadState* _PyThreadState_GET(void) { #if defined(HAVE_THREAD_LOCAL) && !defined(Py_BUILD_CORE_MODULE) return _Py_tss_tstate; #else return _PyThreadState_GetCurrent(); #endif ``` **The caller must hold the GIL** is a problem for us, since the whole idea behind implementing this hook this way is to get the stack when we **don't** have the GIL. This code implements a copy of the current To work around the issue we are copying the Python runtime code, but WITHOUT the GIL checks [_PyFrame_MakeAndSetFrameObject](https://github.com/python/cpython/blob/3.12/Python/frame.c#L28C1-L64C1) is reimplemented to skip the `PyErr_GetRaisedException` call, e.g. [_PyFrame_New_NoTrack](https://github.com/python/cpython/blob/main/Objects/frameobject.c#L2107C1-L2125C2) Changes from ``` PyFrameObject* _PyFrame_New_NoTrack(PyCodeObject *code) { CALL_STAT_INC(frame_objects_created); int slots = code->co_nlocalsplus + code->co_stacksize; PyFrameObject *f = PyObject_GC_NewVar(PyFrameObject, &PyFrame_Type, slots); if (f == NULL) { return NULL; } f->f_back = NULL; f->f_trace = NULL; f->f_trace_lines = 1; f->f_trace_opcodes = 0; f->f_lineno = 0; f->f_extra_locals = NULL; f->f_locals_cache = NULL; f->f_overwritten_fast_locals = NULL; return f; } ``` to ``` thread_local PyFrameObject my_frame; PyFrameObject* MyPyFrame_New_NoTrack(_PyInterpreterFrame* frame) { PyFrameObject* f = &my_frame; if (f == NULL) { return NULL; } f->f_back = NULL; f->f_trace = NULL; f->f_trace_lines = 1; f->f_trace_opcodes = 0; f->f_fast_as_locals = 0; f->f_lineno = 0; f->f_frame = frame; frame->frame_obj = f; Py_SET_TYPE(reinterpret_cast<PyObject*>(frame->frame_obj), &PyFrame_Type); return f; } ``` A thread local variable is being reused since we don't want to keep allocating and deallocating on the heap while doing heap profiling. Reviewed By: fried Differential Revision: D74118996 fbshipit-source-id: 65b9a9a10d6aa26f9c1cd93e30486333f20f4b24
Summary: This diff addresses a problem affecting the interaction of the OCaml runtime, Tokio, and Folly: - Our own `ocamlrep` crate allows us to register finalizers for the `Custom<>` types owned by OCaml. Usually, these just call `drop` on the Rust side. - When an OCaml program terminates, it does not call the finalizers of the objects that are alive at that point (see https://ocaml.org/docs/garbage-collection#finalisation-and-the-weak-module) - When running Hack with `edenfs_file_watcher_enabled`, the server env contains an (opaque) `Custom<EdenfsWatcherInstanceHandle>`, which owns the Tokio runtime executing a worker thread. Due to the points above, when calling `Exit.exit` in the server, the Tokio runtime and worker thread continue running. - Folly registers a low-level hook that is run when the program actually terminates (after the OCaml runtime has finished its own shutdown logic) - This low-level hook can deadlock when the `EdenfsWatcherInstance` is still running. I'm not sure what exactly is causing the deadlock (the fact that the worker thread is still running or the mere existence of the Tokio runtime, ...) Concretely, this problems manifests itself by the server sometimes getting stuck when calling `Exit.exit`. This diff changes the initialization code for `Edenfs_watcher` instances such that whenever we create an instance, we call `Exit.add_hook_upon_clean_exit` to register a hook that will properly shut down the instance. Note that this hook mechanism is part of our own code, and is run before the lower-level exit handler installed by Folly. ## Alternatives considered: OCaml has a "cleanup_on_exit mode, which among other things should call all finalizers (see https://ocaml.org/manual/5.3/runtime.html#s:ocamlrun-options). However, it seems to be buggy in OCaml 5.x (see ocaml/ocaml#10865 (comment)) and running hh_server with `OCAMLRUNPARAM=c` doesn't fix our problem. I'm not sure if we would want to use it anyway, as it may slow down server restarts. # Facebook Here's a stack trace where we get stuck. I've obtained it by attaching gdb to the server process: ``` #0 __futex_abstimed_wait_common64 (private=<optimized out>, cancel=true, abstime=0x0, op=265, expected=3432193, futex_word=0x7fffb4000910) at futex-internal.c:57 #1 __futex_abstimed_wait_common (cancel=true, private=<optimized out>, abstime=0x0, clockid=<optimized out>, expected=3432193, futex_word=0x7fffb4000910) at futex-internal.c:87 facebook#2 __GI___futex_abstimed_wait_cancelable64 (futex_word=0x7fffb4000910, expected=3432193, clockid=<optimized out>, abstime=0x0, private=<optimized out>) at futex-internal.c:139 facebook#3 0x00007ffff729c793 in __pthread_clockjoin_ex (threadid=140736213288512, thread_return=0x0, clockid=0, abstime=0x0, block=<optimized out>) at pthread_join_common.c:105 facebook#4 0x00007ffff7cdf84f in __gthread_join (__value_ptr=0x0, __threadid=<optimized out>) at /home/engshare/third-party2/libgcc/11.x/src/gcc-11.x/x86_64-facebook-linux/libstdc++-v3/include/x86_64-facebook-linux/bits/gthr-default.h:669 facebook#5 std::thread::join (this=0x7fffbe209110) at ../../../.././libstdc++-v3/src/c++11/thread.cc:112 facebook#6 0x00000000062a810d in folly::ThreadPoolExecutor::joinStoppedThreads(unsigned long) () facebook#7 0x00000000062a89ca in folly::ThreadPoolExecutor::stopAndJoinAllThreads(bool) () facebook#8 0x00000000062a395b in folly::IOThreadPoolExecutor::~IOThreadPoolExecutor() () facebook#9 0x00000000062a1975 in folly::IOThreadPoolExecutor::~IOThreadPoolExecutor() () facebook#10 0x0000000006452c09 in folly::detail::SingletonHolder<folly::IOThreadPoolExecutor>::destroyInstance() () facebook#11 0x0000000006fa3c0c in folly::SingletonVault::destroyInstances() () facebook#12 0x00007ffff72478b8 in __run_exit_handlers (status=4, listp=0x7ffff7412658 <__exit_funcs>, run_list_atexit=<optimized out>, run_dtors=<optimized out>) at exit.c:113 facebook#13 0x00007ffff72479ca in __GI_exit (status=<optimized out>) at exit.c:143 facebook#14 0x000000001f401e62 in caml_do_exit (retcode=4) at runtime/sys.c:200 facebook#15 0x000000001f4020dc in caml_sys_exit (retcode=<optimized out>) at runtime/sys.c:205 facebook#16 <signal handler called> facebook#17 0x000000001f33c3f8 in camlStdlib.exit_1534 () at stdlib.ml:580 facebook#18 0x000000001f030ec0 in camlExit.exit_24 () at fbcode/hphp/hack/src/utils/exit.ml:66 facebook#19 0x000000001c2bb13b in camlServerMain.exit_if_critical_update_249 () at fbcode/hphp/hack/src/server/serverMain.ml:108 facebook#20 0x000000001c2bb885 in camlServerMain.query_notifier_549 () at fbcode/hphp/hack/src/server/serverMain.ml:225 facebook#21 0x000000001c2bbe32 in camlServerMain.recheck_until_no_changes_left_916 () at fbcode/hphp/hack/src/server/serverMain.ml:331 ``` So we are shutting down the `folly::IOThreadPoolExecutor`, which then gets stuck waiting for something. A comment inside `__pthread_clockjoin_ex` at the point where we get stucks reads says ``` /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex wake-up when the clone terminates. The memory location contains the thread ID while the clone is running and is reset to zero by the kernel afterwards. The kernel up to version 3.16.3 does not use the private futex operations for futex wake-up when the clone terminates. */ ``` Differential Revision: D76737597 fbshipit-source-id: 979b4e9b3ae88a07fcf62f0958fe41372624d00b
d3e01b9
to
8e3b901
Compare
8e3b901
to
3e5305a
Compare
Thank you for fixing Same goes for including MCRouter in your branch with 423e8b1. This means I can remove this workaround too: if (!\extension_loaded('mcrouter')) {
return new ApcCache(); // cache-like wrapper around apcu
}
// Code below initializes cache-like wrapper around MCRouter, like I do on hhvm 4.168 I also wanted to let you know I am publishing ready-to-use images to Docker Hub, based on your branch. I use them to run my CI. I am going to write about this on my GH page in July. I want to give you all the credit for doing the hard C++ and CMake work. I would like to include a quote of yours about the building of hhvm@next. I can accommodate anything from a single sentence to an entire above-the-fold section, if you'd want to. Here is a quick unrelated Bonus Tip. I don't know if you have already skip Lintian locally. This single threaded part might make up a significant part of your build time. You can disable lintian with this one-line change in make-debianish-package or with this ENV var from debuild. 🙂 |
Summary: Updates typing of function references to polymorphic static methods so that we obtain a polymorphic function type. In particular the typing deals with the following: - Use of method-level generics - Use of class-level generics - `where` constraints - Use of `this` - Use of abstract type constants ## Method-level generics The current type of function references will not return a polymorphic function type. Instead, when referencing a polymorphic method the typing introduces type variables and adds bounds corresponding to any constraints on the generic it replaces. Given the declaration of `bar`: ```[hack] class Wibble<Tclasslevel as arraykey> { public static function bar<Tmethodlevel as arraykey>( Tmethodlevel $y, ): void {} } function refEm(): void { $fptr = Wibble::bar<>; } ``` a fresh type variable will be created with an upper bound of `arraykey` and `$fptr` will be given the type ``` readonly function(#1 $x): #1 ``` where the type variable `#1` has an upper-bound of `arraykey`. Using `$fptr` will be introduce additional constraints on the type variables so the following is currently ill-typed: ```[hack] function expect_int_int((function(int): int) $f): void {} function refEm(): (function(string): string) { $fptr = Wibble::bar<>; expect_int_int($fptr); return $fptr; } ``` After passing `$fptr` to `expect_int_int` the type variable will be solved to `int` so at the return expression we encounter ``` (function(int): int </: function(string): string) ``` With the new typing contained in this diff the function reference will instead be given a polymorphic function type: ``` HH\FunctionRef<(readonly function<Tmethodlevel as arraykey>(Tmethodlevel): Tmethodlevel)> ``` Subtyping of polymorphic function types permits us to pass this function where an expression of type `(function(int): int)` is expected. Doing so does not change the type and we are free to return `$fptr` where an expression of type `(function(string): string)` is expected. Note that this typing is consistent with the already landed typing of generic top-level functions. ## Class-level generics If a static method signature makes use of a generic declared in its enclosing class, it must be moved to the function type. For example: ```[hack] class Wibble<Tclasslevel as arraykey> { public static function foo(Tclasslevel $x): Tclasslevel { return $x; } } function refEm(): (function(string): string) { $fptr = Wibble::foo<>; return $fptr; } ``` The expression `$fptr` will be given the polymorphic function type: ``` HH\FunctionRef<(readonly function<Tclasslevel as arraykey>(Tclasslevel $x): Tclasslevel)> ``` ## `where` constraints A `where` constraint expresses a restriction on a type parameter which may not be defined at the method level e.g. ``` class C<T> { public static function foo(): void where T as arraykey {} } function refEm(): void { $fptr = C::foo<>; } ``` Since `where ` constraints aren't part of the function type, we simplify then discharge any such contraints onto type parameters. This is always possible since we have closed over any required class-level type parameters; in the example we then have: ``` $fptr: HH\FunctionRef<(readonly function<T as arraykey>(): void)> ``` `where` constraints can also relate method-level generics to class-level generics and is further complicated by inheritance. ``` class Base<T> { public static function foo<Tu>(Tu $arg): T where T super vec<Tu> { return vec[$arg]; } } final class Derived<Ta> extends Base<vec<Ta>> {} function refEm(): void { $fptr = Derived::foo<>; } ``` ``` $fptr: HH\FunctionRef<(readonly function<Ta super Tu, Tu as Ta>(Tu $arg): vec<Ta>)> ``` Note that this typing is consistent with the already landed typing of generic top-level functions. ## `this` The type identifier `this` is used to refer to instance types of the lexically enclosing class. The type of function references to static methods which have `this` in their method signature will depend on the variance at which `this` occurs in the method signature. If `this` appears only contravariantly or covariantly, we can directly substitute the `this` for the instance type of the enclosing class, for example ```[hack] class Wibble<Tclasslevel as arraykey> { public static function contra_only(this $_): void{} public static function co_only(): this { ... } } function refEm(): void { $fptr = Wibble::contra_only<>; $gptr = Wibble::co_only<>; } ``` ``` $fptr : HH\FunctionRef<(readonly function<Tclasslevel as arraykey>(Wibble<Tclasslevel> $_): void)> $gptr : HH\FunctionRef<(readonly function<Tclasslevel as arraykey>(): Wibble<Tclasslevel>)> ``` Where `this` occurs invariantly we introduce a type parameter: ```[hack] class Wibble<Tclasslevel as arraykey> { public static function invariantly(this $_): this { ... } } function refEm(): void { $fptr = Wibble::invariantly<>; } ``` ``` $fptr: HH\FunctionRef<(readonly function<Tthis as Wibble<Tclasslevel>, Tclasslevel as arraykey>(Tthis $x): Tthis)> ``` Note that the variance of occurrences of `this` is determined by an analysis of the function signature and is 'aware' of the variance of type parameters in class declarations. In this example, - `this` appears invariantly in the signature of `fizz` since `Box` is invariant in its type parameter - `this` appears covariantly in the signature of `buzz` since `Contra` is contravariant in its type parameter but occurs in a contravariant position whilst `Co` is covariant in its type parameter and occurs in a covariant position. ```[hack] class Box<T> {} class Contra<-T> {} class Co<+T> {} class Wibble<Tclasslevel as arraykey> { public static function fizz(Box<this> $_): void {} public static function buzz(Contra<this> $_): ?Co<this>{ return null; } } ``` The logic for this part of typing is contained in the module `Typing_extract_method.This_variance`. ## Type constants Abstract type constants and concrete type constant aliasing abstract type constants significantly complicate the typing of function references to static methods. The logic for this part of typing is contained in the module `Typing_extract_method.Typeconst_analysis`. In order to 'extract' a method signature involving one of these constants from its containing class we make use of the following equivalence: ``` (exist T. U<T>) -> V === all T. (U<T> -> V) ``` To see how this applies, it's useful to think about the declaration of a class as being implicitly existentially quantified over all abstract type constants it contains. Moving these existentially quantified to type parameters and using class refinements to equate them allows us to be polymorphic in the type constant. For example: ```[hack] abstract class Wibble { abstract const type T as arraykey; public static function foo(this $_, this::T $_, Wibble $_): void {} } function refEm(): void { $fptr = Wibble::foo<>; } ``` ``` HH\FunctionRef<(readonly function<T#0 as arraykey>(Wibble with { type T = T#0 }, T#0, Wibble): void)> ``` Note that in this example, we are only refining the first parameter type (`this`), not the third (`Wibble`). The refinement works for arbitrary access paths, for example ```[hack] abstract class AbstractA { abstract const type TA as arraykey; } abstract class AbstractLeft { abstract const type TLeft as AbstractA; } abstract class AbstractRight { abstract const type TRight as AbstractA; } abstract class AbstractB { abstract const type TL as AbstractLeft; abstract const type TR as AbstractRight; } abstract class AbstractC { abstract const type TC as AbstractB; public static function tickle( this $_, this::TC $_, this::TC::TL $_, this::TC::TR $_, this::TC::TR::TRight $_, this::TC::TL::TLeft $_, this::TC::TR::TRight::TA $_, this::TC::TL::TLeft::TA $_, ): void {} } function refEm(): void { $fptr = AbstractC::tickle<>; } ``` ``` $fptr : HH\FunctionRef<(readonly function< TA0 as arraykey, TA1 as arraykey, TC0 as AbstractB with { type TL = TL0; type TR = TR0 }, TL0 as AbstractLeft with { type TLeft = TLeft0 }, TLeft0 as AbstractA with { type TA = TA0 }, TR0 as AbstractRight with { type TRight = TRight0 }, TRight0 as AbstractA with { type TA = TA1 } >( AbstractC with { type TC = TC0 }, TC0, TL0, TR0, TRight0, TLeft0, TA1, TA0 ): void)> ``` The analysis is also able to correctly type methods with abstract constants containing fix-point refinements: ``` abstract class AbstractA { abstract const type T as AbstractA with { type T = this::T; }; public static function foo(this $_): this::T { throw new Exception(); } } function refEm(): void { $fptr = AbstractA::foo<>; hh_show($fptr); } ``` ``` $fptr: HH\FunctionRef<(readonly function< T#0 as AbstractA with { type T = T#0 } >( AbstractA with { type T = T#0 } ): T#0)> ``` Reviewed By: andrewjkennedy Differential Revision: D74076920 fbshipit-source-id: e4304f2680ac4b3b5d23c42759a867d2ee84b3f0
3e5305a
to
84e2b01
Compare
@hershel-theodore-layton Thank you, I'm glad you find this useful :) I'm honestly not sure what to share about the actual build process. Maybe the single most thing I'm curious about is—who else is out there? AFAIK the largest HHVM users outside Meta are Slack (who have already reached out on the original PR and have been very helpful in hunting down IDE integration issues and other problems), Quizlet and Baidu, but the latter two might well have migrated to Zend at some point, so that leaves me wondering who else stayed on HHVM/Hack. Here follows an assortment of random notes that might be useful for others trying to build HHVM from scratch:
And some aspirational goals/improvements for the build system:
I'm very grateful to the folks from Meta who have been reviewing/landing my PRs split out from the original monstre PR when they find the time for it, so hopefully with time the main PR should get smaller and smaller. Once it's reasonably small, it might be interesting to setup some unofficial periodic OCI image build + publish workflow in a fork or dedicated repo so that people don't have to build their own if they don't want to. As always let me know if you have any questions/suggestions. |
You don't happen to have access to the old hacklang.slack.com Slack instance, right? This used to be the place that both enthusiasts and people who use HHVM at work could join. This is what the Google form at https://hhvm.com/slack was set up for. This Slack is inactive, but it hasn't been removed (yet). |
@hershel-theodore-layton Yeah, I came across the form but I never was in that Slack sadly |
Currently these all use default/target, which seems to cause difficult-to-debug race conditions in OSS CI. Strangely, (locks /cargo) is supposed to prevent this given that locks with an absolute path is documented by dune to be a global lock, but it doesn't seem to work as intended.
The Squangle connection pointer wrapped by AsyncMysqlConnection may be nullptr if the connection was closed or is currently busy waiting for the result of an async query. Most code paths already call either verifyValidConnection() to raise an appropriate Hack exception or explicitly check for and handle a null backing connection, but Query::toString__FOR_DEBUGGING_ONLY() and the SSL-related getters from D33663743 do not, which can lead to segfaults. Slightly simplified reproducer from facebook#8678: ```hack use namespace HH\Lib\SQL; <<__EntryPoint>> async function main_async(): Awaitable<void> { // connection parameters as needed $async_conn = await AsyncMysqlClient::connect('127.0.0.1', 3306, 'foo', 'root', 'pass'); $async_conn->close(); var_dump($async_conn->getSslCertCn()); } async function func_async(AsyncMysqlConnection $asyncMysql, SQL\Query $query): Awaitable<void> { $query->toString__FOR_DEBUGGING_ONLY($asyncMysql); var_dump($asyncMysql->getSslCertCn()); await $asyncMysql->queryf('SELECT %s', 'something'); } ``` and for `(get|is)Ssl.*`: ```hack use namespace HH\Lib\SQL; <<__EntryPoint>> async function main_async(): Awaitable<void> { $async_conn = await AsyncMysqlClient::connect('127.0.0.1', 3306, 'foo', 'root', 'wikia123456'); $async_conn->close(); var_dump($async_conn->getSslCertCn()); } ``` Call verifyValidConnection() in all these cases, as raising an appropriate exception (e.g. closed/busy connection) seems appropriate here. Fixes facebook#8678
Summary: Fix for issue #1 in Post [Truncation of UserMetric::Time](https://fb.workplace.com/groups/560979627394613/permalink/3320975644728317/) Our team uses [BENCHMARK_IMPL_COUNTERS](https://www.internalfb.com/code/fbsource/[f51516f2368d]/fbcode/folly/Benchmark.h?lines=379) to measure custom metrics during Folly::benchmarks execution. While working on updates to NodeAPI::SlaBenchmark in D77320464 I noticed Folly `UserMetric` accepts integer(Long) type values which works great for `UserMetric::Type::Custom` and `UserMetric::Type::Metric`, but causes issues for `UserMetric::Type::Time` values. When [time metrics](https://www.internalfb.com/code/fbsource/[f51516f2368d8097604a13de292824a7d85d7de1]/fbcode/folly/Benchmark.cpp?lines=529) are formated for display the value is implicitly cast(`Long` -> `Double`) resulting in loss of precision. This results in any value smaller than 1s being displayed as 0.00fs due to the truncation making it impossible to display small time values. ~~This diff adds support for users to provide floating point values as input to `UserMetric` allowing precision to be retained through a new `time_value` field, while remaining backward compatible with existing usage that relies on the existing `value` field.~~ See discussion below Reviewed By: yfeldblum Differential Revision: D77319941 fbshipit-source-id: a4d55165b5429fa602ea357764db03a81b9ac760
84e2b01
to
bafab28
Compare
As facebook#9564, but with an attempt to get it to build on a plain GH Actions runner.