feat(ext/fs): add ctime to Deno.stats and use it in node compat layer #24801

lczerniawski · 2024-07-31T06:47:51Z

This PR fixes #24453, by introducing a ctime (using ctime for UNIX and ChangeTime for Windows) to Deno.stats.
The screenshots below show a working example of the code in action.

vs SetMacu tool outputs

lczerniawski · 2024-07-31T06:49:12Z

ext/fs/std_fs.rs

+    Ok(unix_time_msec as u64)
+  }
+
+  const WINDOWS_TICK: i64 = 10_000; // 100-nanosecond intervals in a millisecond


I don't think that this is a good place for consts, but had no idea where to put them, so I will be happy for some suggestions

lczerniawski · 2024-07-31T06:50:35Z

ext/fs/std_fs.rs

+
+    let file_info = {
+      let mut file_info = std::mem::MaybeUninit::<FILE_BASIC_INFO>::zeroed();
+      if GetFileInformationByHandleEx(


Before creating this method, I tried to take the value for ChangeTime from the NtQueryInformationFile used below, but it returns 0.

Are you sure about this? Libuv (which is what Node uses under the hood) gets this information from the NtQueryInformationFile syscall: https://github.com/libuv/libuv/blob/4e310d0f90af29e699e2dedad5fa0dcee181a7cc/src/win/fs.c#L1751-L1793

I think it's quite unfortunate that we would have to do another syscall just to obtain the ctime value.

I'm pretty sure that was the case, but let me check again to make sure.

So I did some digging, and here are my findings, it would be great if someone could check it on their machine, too.

In my case, at least the NtQueryInformationFile doesn't work completely!
I checked the status code for this call. It always returns some big (which might not matter) negative number meaning that it failed to get the data, but on the other hand std::io::Error::last_os_error() returns an object with a status code of 0 and information that it was successful.

So I found that we close a file handle in code before actually using it in the query_file_information method.

but even moving it below the call doesn't change anything at all and NtQueryInformationFile still fails the same way as before.

Now I tried to use the NtQueryInformationFile method in just plain rust program and even tried to get only basic info instead of the full one, but it also failed. I tried that on two different machines with Windows 11 64bit and ARM on board, and it doesn't work in both.

If someone could check on their end whether this is only my machine or if this doesn't seem to work at all, that would be great.

NtQueryInformationFile doesn't set last_os_error() upon failure. Instead, it returns an NTSTATUS code and you have to convert it to a win32 error (using RtlNtStatusToDosError) manually to know what the error was.

Good to know, thanks, I assumed this cause that's how handling is done in deno code right now. I will have a look at this then, what about the CloseHandle placement, shouldn't it be after each call that uses the handle?

It looks like the call to RtlNtStatusToDosError is entirely missing from the current code base, which means that when NtQueryInformationFile fails we are not currently reporting an appropriate error code.

Thank you so much for your help! I'm not an expert on a Windows API topic (it is my first time touching it), but it was a lot of fun to learn about it! 😄. I managed to fix the NtQueryInformationFile call and get all the data from a single call, and I have added some proper error handling. Let me know if there is something else that can be improved.

lczerniawski · 2024-08-02T15:41:44Z

ext/fs/std_fs.rs

@@ -862,10 +883,11 @@ fn stat_extra(
    }

    let result = get_dev(file_handle);
-    CloseHandle(file_handle);


This one needs to be moved after we use the handle in NtQueryInformationFile otherwise ERROR_NOACCESS(998) is returned

lczerniawski · 2024-08-02T15:42:02Z

ext/fs/std_fs.rs

    let status = NtQueryInformationFile(
      handle as _,
-      std::ptr::null_mut(),
+      io_status_block.as_mut_ptr(),


With null ptr it will crash resulting in ERROR_NOACCESS (998) code

lczerniawski · 2024-08-02T15:49:09Z

ext/fs/std_fs.rs

+      // since struct is populated with other data anyway.
+      /// https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntqueryinformationfile#remarks
+
+      if converted_status != ERROR_MORE_DATA {


According to Microsoft docs (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntqueryinformationfile#remarks) it is fine to ignore it as long as we don't care about full file name "If NtQueryInformationFile fails because of a buffer overflow, drivers that implement FileNameInformation should return as many WCHAR characters of the file name as will fit in the buffer and specify the full length that is required in the FileNameLength parameter of the FILE_NAME_INFORMATION structure.". To make it work without an error, we would have to query the sys call again with size buffer of struct + length of the file name, as the FILE_ALL_INFORMATION has variable size in FILE_NAME_INFORMATION struct (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddk/ns-ntddk-_file_name_information)

lczerniawski · 2024-08-03T09:57:33Z

ext/fs/30_fs.js

    ino: unix ? response.ino : null,
-    mode: unix ? response.mode : null,


To explain why I removed that, I don't think this is true anymore; after a fix in this PR which now results in `NtQueryInformationFile' working correctly, we indeed set the mode in std_fs.rs lines 880/883/891/894.

This part looks fairly independent of the main change of this PR. Can you split this to another PR?

Like the whole .js file change? Yeah, I can do that if you think it would be better. For me, it is more of a whole as by changing the underlying Rust structure, we should also update the JS API, but if you wish otherwise, sure, I can :)

Like the whole .js file change?

I meant only split mode value change. I think the change of mode value can be done independently from ctime addition, or am I missing something?

Well, I had to double-check, but as I suspected I done it cause the tests start to fail

https://github.com/denoland/deno/actions/runs/11594876322/job/32282184345?pr=24801

It assumes that for Windows mode should be empty, but after my fix to NtQueryInformationFile call, it is no longer true (and the whole code for setting mode was already there I didn't change that in any way).

So it is either changing the tests, but then tests will accommodate incorrect behavior or changing what we expose in API, honestly I can do both, but for me personally it makes more sense to just do it once and fix as it should be, but of course please let me know how you view this thing after my explanation, for now I left the code with rollbacked version of changes that fails the tests

kt3k · 2024-11-01T10:17:03Z

Looks like FsInfo.mode is supposed to be existing on windows since #24434, but actually isn't (and this PR seems fixing it by moving CloseHandle call). I'll rollback this branch to the earlier state. Sorry for the confusion! @lczerniawski

This reverts commit 90b233a.

This reverts commit a2814b8.

lczerniawski · 2024-11-04T11:03:21Z

Looks like FsInfo.mode is supposed to be existing on windows since #24434, but actually isn't (and this PR seems fixing it by moving CloseHandle call). I'll rollback this branch to the earlier state. Sorry for the confusion! @lczerniawski

No problem! That's what I meant, but I guess I didn't express myself particularly well 😄

kt3k

LGTM. Thanks for your work!

Because this includes a new feature to Deno API, we are going to land this for Deno 2.1. The merge window for 2.1 is planned to be opened next week.

lczerniawski added 18 commits July 29, 2024 15:32

Add ctime to FsStat and support of it for unix

61b0818

Add ctime support for windows, based on ChangeTime field

6dc80bf

Convert ctim to milliseconds

86933ee

Expose ctime to TS interface

a939477

Fix node compact layer ctime to use ctime instead of mtime

789e7dd

Update windows to get ctime from special method

60c1305

Fix ctime pass to struct

18bd277

Fix missing method

e93a521

Fix missing std and using

f1f74d1

Fix missing &

2b51e87

Fix wrong borrowed value

83eb7aa

Move around helper function for time conversion

dd69f76

Add error handling for both unix and windows

054c7b1

Fix missing ctime in js struct

9fb0be7

Fix formating

ea7db6f

Change the way that get_change_time for windows is structured

452b295

Remove unnecessary mutable

7cd2a6d

Remove unnecessary variable

eed2a7a

lczerniawski commented Jul 31, 2024

View reviewed changes

lucacasonato requested a review from piscisaureus July 31, 2024 09:53

Fix NtQueryInformationFile and get ChangeTime from single sys call

6d48778

lczerniawski commented Aug 2, 2024

View reviewed changes

Add a Microsoft source to comment

68b5dd6

lczerniawski commented Aug 2, 2024

View reviewed changes

lczerniawski added 2 commits August 2, 2024 17:50

Add a Microsoft source to comment

ba32d4a

Fix lint and format

0791b32

lczerniawski changed the title ~~fix(node compat): Add ctime to Doen.stats and use it in node compact layer~~ fix(node compat): Add ctime to Deno.stats and use it in node compact layer Aug 2, 2024

Fix comment

de29ab2

lczerniawski added 9 commits August 2, 2024 19:50

Fix missing change mtime -> ctime Node polyfills

d4c1052

Add tests for ctime

98dd346

Add tests for ctime

e66d2c9

Remove mode unix only filter in parseFileInfo

2df81fc

Fix lint

6ee7cf4

Remove ctime from fstat test

a734201

Make mode non-optional as it is now working on Windows

67a581c

Move mode prop

c4b2e17

Rollback props positions

2fae386

lczerniawski commented Aug 3, 2024

View reviewed changes

Merge branch 'main' into addCtimeToStats

2223bec

bartlomieju assigned kt3k Oct 29, 2024

lczerniawski and others added 2 commits October 30, 2024 15:05

Rollback mode changes

a2814b8

make mode optional in fs stat struct

90b233a

kt3k changed the title ~~fix(node compat): Add ctime to Deno.stats and use it in node compact layer~~ feat(ext/fs): add ctime to Deno.stats and use it in node compat layer Nov 1, 2024

kt3k added 3 commits November 1, 2024 20:27

Revert "make mode optional in fs stat struct"

dff37a7

This reverts commit 90b233a.

Revert "Rollback mode changes"

4230868

This reverts commit a2814b8.

test: check info.mode exists on windows

33b21f5

fix assertion

299cb34

kt3k force-pushed the addCtimeToStats branch from ccb01ad to 299cb34 Compare November 5, 2024 11:25

kt3k approved these changes Nov 5, 2024

View reviewed changes

Merge branch 'main' into addCtimeToStats

5da8465

kt3k merged commit 7becd83 into denoland:main Nov 13, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ext/fs): add ctime to Deno.stats and use it in node compat layer #24801

feat(ext/fs): add ctime to Deno.stats and use it in node compat layer #24801

lczerniawski commented Jul 31, 2024

lczerniawski Jul 31, 2024

lczerniawski Jul 31, 2024

piscisaureus Jul 31, 2024

lczerniawski Jul 31, 2024

lczerniawski Aug 1, 2024 •

edited

Loading

piscisaureus Aug 1, 2024

lczerniawski Aug 1, 2024

piscisaureus Aug 1, 2024

lczerniawski Aug 3, 2024

lczerniawski Aug 2, 2024

lczerniawski Aug 2, 2024

lczerniawski Aug 2, 2024

lczerniawski Aug 3, 2024

kt3k Oct 28, 2024

lczerniawski Oct 30, 2024

kt3k Oct 30, 2024

lczerniawski Oct 30, 2024

kt3k commented Nov 1, 2024

lczerniawski commented Nov 4, 2024

kt3k left a comment

		ino: unix ? response.ino : null,
		mode: unix ? response.mode : null,

feat(ext/fs): add ctime to Deno.stats and use it in node compat layer #24801

feat(ext/fs): add ctime to Deno.stats and use it in node compat layer #24801

Conversation

lczerniawski commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lczerniawski Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kt3k commented Nov 1, 2024

lczerniawski commented Nov 4, 2024

kt3k left a comment

Choose a reason for hiding this comment

lczerniawski Aug 1, 2024 •

edited

Loading