Skip to content
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

Release proposal: v1.39.0 #2943

Closed
cjihrig opened this issue Aug 4, 2020 · 15 comments
Closed

Release proposal: v1.39.0 #2943

cjihrig opened this issue Aug 4, 2020 · 15 comments

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2020

Notable changes:

  • uv_metrics_idle_time() and UV_METRICS_IDLE_TIME have been added for measuring the amount of time the event loop spends idle.
  • uv_udp_using_recvmmsg() has been added to determine if a buffer is large enough for multiple datagrams should be allocated in the allocation callback of uv_udp_recvstart().
  • On MinGW, the installation location has been updated to match Unix systems rather than Windows.
  • uv_fs_copyfile() now tries to use copy_file_range() when possible.
  • The test suite is now reported to pass on Darwin ARM64 (Apple Silicon).
  • uv_{get,set}_process_title() now returns an error on platforms where uv_setup_args() is required, but has not yet been called.
  • The _POSIX_PATH_MAX constant is no longer used, which could lead to buffer overflows in uv_fs_readlink() and uv_fs_realpath().
@vtjnash
Copy link
Member

vtjnash commented Aug 4, 2020

There's probably a handful of other PRs nearly ready to merge also, but waiting on either a quick correction or review!

@bnoordhuis
Copy link
Member

Is #2725 really ready for release? It's a new API and those should ideally receive at least two sign-offs from maintainers.

Although it received three, it looks like two of them are from March, for an old incarnation of the PR.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 4, 2020

@bnoordhuis I guess that's a good question. I'm fine with waiting on a release while that is determined.

@trevnorris
Copy link
Member

@bnoordhuis I've uploaded the diff from the first accepted patch in March and the one that landed in v1.x to https://gist.github.com/trevnorris/8183ea638732f908ebb69f962e50b4e0.

I don't have any issue with waiting, but for posterity I want to point out that there was almost no change between the patch as it was accepted in March and as it landed today. The only major differences between the two are a mistake that accidentally broke ABI:

@@ include/uv.h: typedef struct uv_utsname_s uv_utsname_t;
  typedef enum {
 -  UV_LOOP_BLOCK_SIGNAL
-+  UV_LOOP_BLOCK_SIGNAL = 1,
-+  UV_METRICS_IDLE_TIME = 2
++  UV_LOOP_BLOCK_SIGNAL = 0,
++  UV_METRICS_IDLE_TIME
  } uv_loop_option;

and a bug fix for calculating idle time in Windows.

@@ src/win/core.c: static void uv__poll_wine(uv_loop_t* loop, DWORD timeout) {
 +      timeout = user_timeout;
 +      reset_timeout = 0;
 +    }
++
++    /* Placed here because on success the loop will break whether there is an
++     * empty package or not, or if GetQueuedCompletionStatus returned early then
++     * the timeout will be updated and the loop will run again. In either case
++     * the idle time will need to be updated.
++     */
++    uv__metrics_update_idle_time(loop);
 +
      if (overlapped) {
-+      uv__metrics_update_idle_time(loop);
@@ src/win/core.c: static void uv__poll(uv_loop_t* loop, DWORD timeout) {
 +    if (reset_timeout != 0) {
 +      timeout = user_timeout;
 +      reset_timeout = 0;
 +    }
++
++    /* Placed here because on success the loop will break whether there is an
++     * empty package or not, or if GetQueuedCompletionStatus returned early then
++     * the timeout will be updated and the loop will run again. In either case
++     * the idle time will need to be updated.
++     */
++    uv__metrics_update_idle_time(loop);
 +
      if (success) {
        for (i = 0; i < count; i++) {
          /* Package was dequeued, but see if it is not a empty package
           * meant only to wake us up.
           */
          if (overlappeds[i].lpOverlapped) {
-+          uv__metrics_update_idle_time(loop);

@bnoordhuis
Copy link
Member

Okay, that seems harmless enough, although I can't really evaluate the windows changes.

@musm
Copy link
Contributor

musm commented Aug 6, 2020

Any chance #2910 can make it?

@ulrikstrid
Copy link
Contributor

If there was is any chance I would love it if #2951 got in as well. Let me know if there's anything I can do to make it happen.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 9, 2020

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 14, 2020

Small status update. The changes in this release are causing some problems with the Node.js test suite:

  1. 07e4168 requires some additional calls to uv_setup_args() in the Node test suite. Addressed in test,doc: add missing uv_setup_args() calls nodejs/node#34751.
  2. 12be29f (specifically, the change to uv_shutdown()) is causing Node's test/parallel/test-http2-buffersize.js test to become extremely flaky on macOS. Still investigating this one.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 24, 2020

@bnoordhuis do you have any ideas on item 2 above?

@bnoordhuis
Copy link
Member

12be29f (specifically, the change to uv_shutdown()) is causing Node's test/parallel/test-http2-buffersize.js test to become extremely flaky on macOS.

I'm not quite sure why but it's the clearing of the flags in the error path that causes the flakiness. With the patch below, the test starts passing again:

diff --git a/src/unix/stream.c b/src/unix/stream.c
index f86cf11e..6c0f5ad1 100644
--- a/src/unix/stream.c
+++ b/src/unix/stream.c
@@ -1189,7 +1189,6 @@ static void uv__read(uv_stream_t* stream) {
 #endif
       } else {
         /* Error. User should call uv_close(). */
-        stream->flags &= ~(UV_HANDLE_READABLE | UV_HANDLE_WRITABLE);
         stream->read_cb(stream, UV__ERR(errno), &buf);
         if (stream->flags & UV_HANDLE_READING) {
           stream->flags &= ~UV_HANDLE_READING;

Suggestion: revert the commit for now so we can move forward with the release? I'll open a pull request.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 24, 2020

Partial revert in #2967

Revert in #2968

cjihrig added a commit to cjihrig/libuv that referenced this issue Aug 24, 2020
This reverts commit 12be29f.

The commit in question was introducing failures in the Node.js
test suite.

Refs: libuv#2943
bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Aug 24, 2020
This reverts commit 12be29f.

Reverted for making `test/parallel/test-http2-buffersize.js` from the
Node.js test suite turn *really* flaky.

Refs: libuv#2943 (comment)
cjihrig added a commit that referenced this issue Aug 24, 2020
This reverts commit 12be29f.

The commit in question was introducing failures in the Node.js
test suite.

Refs: #2943
Refs: #2967
Refs: #2409
PR-URL: #2968
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 24, 2020

libuv CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/2019/
Node.js + libuv integration CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/161/

I'll cut the new release once those CI runs come back.

EDIT: Another Node integration CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/162/. Combined with the first run, it's green.
EDIT: Another libuv CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/2022/.

@vtjnash
Copy link
Member

vtjnash commented Nov 13, 2020

Hm, I should have saved the logs from that error. Anyone know what the reported flakiness error was? Otherwise, I'm tempted just to re-merge that commit, since we don't have any counter-examples on CI anymore indicating to do otherwise.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 13, 2020

I don't remember what the error was, unfortunately.

vtjnash added a commit that referenced this issue May 21, 2021
This reverts commit 46f36e3.

PR-URL: #3006
Refs: #2967
Refs: #2409
Refs: #2943
Refs: #2968
Refs: nodejs/node#36111
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
JeffroMF pushed a commit to JeffroMF/libuv that referenced this issue May 16, 2022
This reverts commit 12be29f.

The commit in question was introducing failures in the Node.js
test suite.

Refs: libuv#2943
Refs: libuv#2967
Refs: libuv#2409
PR-URL: libuv#2968
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
JeffroMF pushed a commit to JeffroMF/libuv that referenced this issue May 16, 2022
JeffroMF pushed a commit to JeffroMF/libuv that referenced this issue May 16, 2022
This reverts commit 46f36e3.

PR-URL: libuv#3006
Refs: libuv#2967
Refs: libuv#2409
Refs: libuv#2943
Refs: libuv#2968
Refs: nodejs/node#36111
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants