Fixes bug in commit 25df2b3f11.
The code for converting FSEventStream notification into Dart format
was checking file existence using relative path instead of absolute
path. This caused it to incorrectly classify rename events which
were creations as deletions.
This was not caught by testing because this was not well covered
by the test and the test closest to checking this was even
disabled on Mac. CL updates updates the test to cover this better.
Fixes https://github.com/dart-lang/sdk/issues/61693
TEST=standalone/io/file_system_watcher_test
Change-Id: I2a7c984dfcec0be74c974f13b6ead7d86a6a6964
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/453880
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Watcher used pipes to fit into the "socket"-like implementation used by
other OSes. However it turns out that pipe buffer on Mac OS X is not
necessarily large enough to fit the whole FSEvent structure meaning that
we can't expect to be able to read FSEvents atomically from a pipe.
This situation happens because Mac OS X inherits BSD behavior of
limiting total number of kernel memory reserved for pipe buffers. Once
kern.ipc.maxpipekva limit is crossed pipe buffers no longer grow to 64KB
and remain 512 bytes large (which is absolute minimum allowed by POSIX
which requires writes smaller than PIPE_BUF to be atomic).
So our code needs to be prepared that only half of FSEvent structure
(which is 1032 bytes large) will fit into pipe buffer.
We could fix this by reading each FSEvent in chunks but this seems
ridiculously ineffecient. Instead we rewrite the code to pass file
system events via SendPort instead.
Fixes https://github.com/dart-lang/sdk/issues/61551
TEST=runtime/tests/vm/dart/regress_61551_test.dart
CoreLibraryReviewExempt: VM only changes.
Change-Id: I6a6a696499044832cfb61998bc6a519bbaadf8bc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/451260
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Existing implementation is an entangled mess which consists of shared
code residing in the base class which in random places invokes a number
of undocumented poorly named methods overloaded in OS specific
subclasses. Some of these methods mutate static state. There are no
clear lifetime guarantees for different parts of the system (including
comments saying that some values might or might not be valid at certain
points).
The rewrite aims to clean most of this up - sharing everything that can
be shared and moving OS specific logic to clearly documented methods.
Furthermore, we change the code to ensure proper lifetime guarantees -
so we no longer find ourself in situations where we don't know whether
pathId is valid or not.
This refactoring by itself fixes a number of issues, most specifically a
bug where watcher would stop receiving events on Windows because
DirectoryWatchHandle ends up allocated at precisely the same address as
a previous destroyed one - which confuses Dart side to think that newly
created handle is the same as the old one (due to a race between event
handler thread and Dart thread).
We fix Windows lifetime issue by a) not keeping pathId based mapping in
the watcher anymore and b) keeping DirectoryWatchHandler alive until it
is stoped by the Dart side - this is achieved by retaining it after it
is created and releasing it once path is unwatched. This way Dart side
is always sure that pathId values are valid until they are explicitly
released via _unwatchPath - which makes code very uniform.
To make sure that native objects created by _watchPath are released when
surrounding isolate exists abruptly (e.g. via Isolate.exit - without
letting Dart code to shutdown and call _unwatchPath naturally) we attach
NativeFinalizer to them. This fixes the existing leak of file watchers
on Mac OS X - as Node objects it created were not freed if surrounding
isolate exited. Note that inotify descriptors did not leak in the same
way because they were wrapped into sockets.
Finally, this refactoring also make sure that the last subscriber
cancelling subscription on filesystem event stream will get a proper
cancellation future back and can wait for the watcher to shutdown.
Previously implementation used broadcast streams which simply return an
already completed future when subscriber cancels. New implementation
uses Stream.multi instead which gives a better result. Now doing
watch().listen().cancel() returns a future which will only complete once
watcher is fully disposed (e.g. inotify descriptor is closed). Bad
behavior was revealed by analysing standalone/regress_52715 - which
revealed that repeatedly watching and cancelling might flakely cause us
to hit fd limit depending on whether eventhandler thread can keep up
closing file descriptors created by the main thread or not.
Fixes https://github.com/dart-lang/sdk/issues/61378
TEST=standalone/{regress_61378,file_system_watcher_isolate_exit_leak}
CoreLibraryReviewExempt: VM only changes.
Change-Id: I6a6a69642b1f2673f2be78434bc64270846ad8c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/450921
Reviewed-by: Lasse Nielsen <lrn@google.com>
This CL also removes ability to assign names to mutexes which was
added in c25ebfff96 but did not yield
any interesting data.
TEST=ci
CoreLibraryReviewExempt: Changes to dart:concurrent only.
Cq-Include-Trybots: luci.dart.try:vm-fuchsia-release-x64-try,vm-win-debug-x64-try,vm-win-release-x64-try,vm-aot-linux-product-x64-try
Change-Id: Id41e1d29832f6008e02f0a571ee67564e1a84224
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/375300
Reviewed-by: Martin Kustermann <kustermann@google.com>
This relands https://dart-review.googlesource.com/c/sdk/+/205633
but without renaming TARGET_OS_IPHONE to DART_TARGET_OS_IPHONE.
It also changes uses of TARGET_OS_IOS to
DART_TARGET_OS_MACOS_IOS to be consistent with the rest of the
VM.
TargetConditionals.h for XCode 13 defines several
TARGET_OS_* preprocessor symbols that confuse the
Dart build. There is probably a more targeted fix
for this, but renaming the symbols that Dart uses
will also prevent this problem if more symbols
are added to the platform headers in the future.
See: https://github.com/dart-lang/sdk/issues/46499
TEST=It builds.
Change-Id: Ie775c19dd23cfdf5f65e5ebc6ee4ec3a561676fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/205860
Commit-Queue: Zach Anderson <zra@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
TargetConditionals.h for XCode 13 defines several
TARGET_OS_* preprocessor symbols that confuse the
Dart build. There is probably a more targeted fix
for this, but renaming the symbols that Dart uses
will also prevent this problem if more symbols
are added to the platform headers in the future.
See: https://github.com/dart-lang/sdk/issues/46499
TEST=It builds.
Change-Id: I3b33a03b4a9a14b76d55fe12f8cdefec4b3c3664
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/205633
Commit-Queue: Zach Anderson <zra@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
At present start/stop requests are scheduled on RunLoop thread.
This results in deadlocks since same RunLoop thread might be busy
with blocking writes of file watching events, not giving a chance
for Dart to read previously-written events. Reading would unblock
writer.
So this CL moves start/stop requests to run on Dart thread instead.
Fixes https://github.com/dart-lang/sdk/issues/45996
TEST=run analysis_server against flutter_gallery github-backed folder, switch between branches and ensure analyze_server remains responsive
Change-Id: I0464eeecf8e46ba3027fa0ed21cc323495d965c3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201442
Commit-Queue: Alexander Aprelev <aam@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Fuchsia requires the ability to sandbox Isolates w.r.t. file IO.
When a new Isolate starts, Fuchsia will pass the Isolate an object
called a namespace. We can translate the namespace object into a
file descriptor suitable for passing to the *at() family of
POSIX file system calls. The file system calls will then
have visibility only into the specified namespace.
We also plumb Namespaces through on all the other platforms as well to
make the change easier to test and so that in the future we can
implement e.g. per-isolate cwds.
This change adds a new internal class to dart:io called _Namespace,
which is implemented in a patch file. See:
sdk/lib/io/namespace_impl.dart
runtime/bin/namespace_patch.dart
The embedder can set up a non-default namespace by calling
_Namespace._setupNamespace during Isolate setup.
Instances of _Namespace have a native field that holds a pointer
to a native Namespace object. See:
runtime/bin/namespace.h
Calls from e.g. file_impl.dart are now also passed a
_Namespace object. The implementations in e.g. file.cc and
file_linux.cc then extract the namespace, and use it to compute a
file descriptor and path suitable for passing to e.g. openat().
related US-313
R=asiva@google.com, rmacnak@google.com
Review-Url: https://codereview.chromium.org/3007703002 .
Like HOST_ARCH_*, HOST_OS_* describes the OS the VM is running on, which may be different from the OS the VM is generating code for during AOT compilation.
Currently we conflate the two when emitting AOT as assembly, and we get away with it because Flutter only uses assembly for targeting iOS and one can only target iOS from a Mac, but we expect to use assembly for Android as well so native tools can unwind Dart frames.
R=zra@google.com
Review-Url: https://codereview.chromium.org/2750843003 .
Previously we would compile in implementaitions of native calls for
IO functions that would never be used. This CL provides implementations
that throw a Dart exception if they're called by mistake. It also uses
a DART_IO_DISABLED preprocessor define to clean up the build files and
check that we're not including code we shouldn't.
R=iposva@google.com, johnmccutchan@google.com
Review URL: https://codereview.chromium.org/1839463002 .
Alexander Aprelev