Permalink
Browse files

Logging to track Relative_path.Dummy showing up in unexpected places

Summary:
The assertion in errors.ml is (rarely, compared to memory issues, but still) being hit in production. That means that at some point we are thinking that there are definitions in Dummy ("default") path, which is likely very bad for our incremental mode.

Things should have default path only when we are analyzing something "on the side" for the purposes of IDE, and should not leak outside of this computation thanks to ServerIdeUtils.make_then_revert_local_changes.

They do somehow leak out, or are created elsewhere, and in this diff I add checks in few more places where they should not show up to narrow down how does it happen:

- log when there no local changes (i.e we are not inside make_then_revert_local_changes) but we add a default path to naming heap
- log when there are no local changes and we get the default path back from TypeIdHeap (the only heap with cache). If this happens, try bypassing the cache to check if it matches the actual heap and log if it doesn't
- log when default path shows up in "files to recheck" in ServerTypecheck
- log in which phase are the "bad" errors created

Reviewed By: gregory-nisbet

Differential Revision: D7334282

fbshipit-source-id: 285d076073946ce81e83dd018c6f6e4021003cdb
  • Loading branch information...
dabek authored and hhvm-bot committed Mar 23, 2018
1 parent 11dee3e commit afb24061be44a255a0fb410d7cf913a49358bafa
@@ -377,6 +377,7 @@ module Raw (Key: Key) (Value:Value.Type): sig
val move : Key.md5 -> Key.md5 -> unit
module LocalChanges : sig
val has_local_changes : unit -> bool
val push_stack : unit -> unit
val pop_stack : unit -> unit
val revert : Key.md5 -> unit
@@ -466,6 +467,8 @@ end = struct
let stack: t option ref = ref None
let has_local_changes () = Option.is_some (!stack)
let rec mem stack_opt key =
match stack_opt with
| None -> hh_mem key
@@ -741,6 +744,7 @@ module type NoCache = sig
val revive_batch : KeySet.t -> unit
module LocalChanges : sig
val has_local_changes : unit -> bool
val push_stack : unit -> unit
val pop_stack : unit -> unit
val revert_batch : KeySet.t -> unit
@@ -753,6 +757,7 @@ end
module type WithCache = sig
include NoCache
val write_through : key -> t -> unit
val get_no_cache : key -> t option
end
(*****************************************************************************)
@@ -1082,6 +1087,8 @@ module WithCache (UserKeyType : UserKeyType) (Value:Value.Type) = struct
Direct.add x y;
Cache.add x y
let get_no_cache = Direct.get
let write_through x y =
(* Note that we do not need to do any cache invalidation here because
* Direct.add is a no-op if the key already exists. *)
@@ -1162,5 +1169,8 @@ module WithCache (UserKeyType : UserKeyType) (Value:Value.Type) = struct
let commit_all () =
Direct.LocalChanges.commit_all ();
Cache.clear ()
let has_local_changes () =
Direct.LocalChanges.has_local_changes ()
end
end
@@ -199,6 +199,7 @@ module type NoCache = sig
* the shared memory until the local changes are popped off.
**)
module LocalChanges : sig
val has_local_changes : unit -> bool
(** Push a new local change environment **)
val push_stack : unit -> unit
(** Pop off the last local change environment **)
@@ -219,6 +220,7 @@ end
module type WithCache = sig
include NoCache
val write_through : key -> t -> unit
val get_no_cache : key -> t option
end
module type UserKeyType = sig
@@ -26,23 +26,51 @@ module FunCanonHeap : CanonHeap = SharedMem.NoCache (StringKey) (struct
let description = "FunCanon"
end)
let check_valid key pos =
if FileInfo.get_pos_filename pos = Relative_path.default then begin
Hh_logger.log
("WARNING: setting canonical position of %s to be in dummy file. If this \
happens in incremental mode, things will likely break later.") key;
Hh_logger.log "%s"
(Printexc.raw_backtrace_to_string (Printexc.get_callstack 100));
end
(* TypeIdHeap records both class names and typedefs since they live in the
* same namespace. That is, one cannot both define a class Foo and a typedef
* Foo (or FOO or fOo, due to case insensitivity). *)
module TypeIdHeap = SharedMem.WithCache (StringKey) (struct
type t = FileInfo.pos * [`Class | `Typedef]
let prefix = Prefix.make ()
let description = "TypeId"
end)
module TypeIdHeap = struct
include SharedMem.WithCache (StringKey) (struct
type t = FileInfo.pos * [`Class | `Typedef]
let prefix = Prefix.make ()
let description = "TypeId"
end)
let add x y =
if not @@ LocalChanges.has_local_changes () then check_valid x (fst y);
add x y
module FunPosHeap = SharedMem.NoCache (StringKey) (struct
type t = FileInfo.pos
let prefix = Prefix.make()
let description = "FunPos"
end)
let write_through x y =
if not @@ LocalChanges.has_local_changes () then check_valid x (fst y);
write_through x y
end
module ConstPosHeap = SharedMem.NoCache (StringKey) (struct
type t = FileInfo.pos
let prefix = Prefix.make()
let description = "ConstPos"
end)
module FunPosHeap = struct
include SharedMem.NoCache (StringKey) (struct
type t = FileInfo.pos
let prefix = Prefix.make()
let description = "FunPos"
end)
let add x y =
if not @@ LocalChanges.has_local_changes () then check_valid x y;
add x y
end
module ConstPosHeap = struct
include SharedMem.NoCache (StringKey) (struct
type t = FileInfo.pos
let prefix = Prefix.make()
let description = "ConstPos"
end)
let add x y =
if not @@ LocalChanges.has_local_changes () then check_valid x y;
add x y
end
@@ -741,6 +741,8 @@ end = functor(CheckKind:CheckKindType) -> struct
ServerCheckpoint.process_updates fast;
debug_print_fast_keys genv "to_recheck" fast;
debug_print_path_set genv "lazy_check_later" lazy_check_later;
if Relative_path.(Map.mem fast default) then
Hh_logger.log "WARNING: recheking defintion in a dummy file";
let errorl'=
Typing_check_service.go genv.workers env.tcopt fast in
let errorl' = match ServerArgs.ai_mode genv.options with
@@ -10,14 +10,34 @@
open Typing_heap
let check_cache_consistency x expected_kind expected_result =
if
expected_result = Relative_path.default &&
(not @@ Naming_heap.TypeIdHeap.LocalChanges.has_local_changes ())
then begin
Hh_logger.log "WARNING: found dummy path in shared heap for %s" x;
match Naming_heap.TypeIdHeap.get_no_cache x with
| Some (pos, kind) when kind = expected_kind &&
FileInfo.get_pos_filename pos = expected_result -> ()
| _ ->
Hh_logger.log "WARNING: get and get_no_cache returned different results";
end
let get_type_id_filename x expected_kind =
match Naming_heap.TypeIdHeap.get x with
| Some (pos, kind) when kind = expected_kind ->
let res = FileInfo.get_pos_filename pos in
check_cache_consistency x expected_kind res;
Some res
| _ -> None
let get_class tcopt x =
match Classes.get x with
| Some c ->
Some c
| None ->
match Naming_heap.TypeIdHeap.get x with
| Some (pos, `Class) ->
let filename = FileInfo.get_pos_filename pos in
match get_type_id_filename x `Class with
| Some filename ->
Errors.run_in_decl_mode filename
(fun () -> Decl.declare_class_in_file tcopt filename x);
Classes.get x
@@ -51,9 +71,8 @@ let get_typedef tcopt x =
match Typedefs.get x with
| Some c -> Some c
| None ->
match Naming_heap.TypeIdHeap.get x with
| Some (pos, `Typedef) ->
let filename = FileInfo.get_pos_filename pos in
match get_type_id_filename x `Typedef with
| Some filename ->
Errors.run_in_decl_mode filename
(fun () -> Decl.declare_typedef_in_file tcopt filename x);
Typedefs.get x
@@ -60,7 +60,7 @@ let files_t_merge ~f x y =
Relative_path.Map.merge x y ~f:begin fun k x y ->
let x = Option.value x ~default:PhaseMap.empty in
let y = Option.value y ~default:PhaseMap.empty in
Some (PhaseMap.merge x y ~f:(fun _k x y -> f k x y))
Some (PhaseMap.merge x y ~f:(fun phase x y -> f phase k x y))
end
let files_t_to_list x =
@@ -489,7 +489,7 @@ and add_list code pos_msg_l =
add_ignored_fixme_code_error pos code
and merge (err',fixmes') (err,fixmes) =
let append = fun _ x y ->
let append = fun _ _ x y ->
let x = Option.value x ~default: [] in
let y = Option.value y ~default: [] in
Some (List.rev_append x y)
@@ -527,13 +527,21 @@ and incremental_update :
| Some x -> Relative_path.Map.add acc path x
in
(* Replace old errors with new *)
let res = files_t_merge old new_ ~f:begin fun path old new_ ->
if path = Relative_path.default then
let res = files_t_merge old new_ ~f:begin fun phase path old new_ ->
if path = Relative_path.default then begin
let phase = match phase with
| Parsing -> "Parsing"
| Naming -> "Naming"
| Decl -> "Decl"
| Typing -> "Typing"
in
Utils.assert_false_log_backtrace (Some(
"Default (untracked) error sources should not get into incremental " ^
"mode. There might be a missing call to Errors.do_with_context/" ^
"run_in_context somwhere or incorrectly used Errors.from_error_list"
));
"run_in_context somwhere or incorrectly used Errors.from_error_list." ^
"Phase: " ^ phase
))
end;
match new_ with
| Some new_ -> Some (List.rev new_)
| None -> old

0 comments on commit afb2406

Please sign in to comment.