You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/10/05 02:20:21 UTC

[GitHub] [apisix] membphis opened a new pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

membphis opened a new pull request #2357:
URL: https://github.com/apache/apisix/pull/2357


   
   
   ### What this PR does / why we need it:
   <!--- Why is this change required? What problem does it solve? -->
   <!--- If it fixes an open issue, please link to the issue here. -->
   
   related PR: https://github.com/apache/apisix/pull/2306
   
   ### Pre-submission checklist:
   
   * [ ] Did you explain what problem does this PR solve? Or what new features have been added?
   * [ ] Have you added corresponding test cases?
   * [ ] Have you modified the corresponding document?
   * [ ] Is this PR backward compatible?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] moonming commented on a change in pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#discussion_r501085354



##########
File path: apisix/init.lua
##########
@@ -82,6 +75,14 @@ end
 
 
 function _M.http_init_worker()
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       Can it help user to find any problems?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] moonming commented on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703370308


   why only `init` phase is not enough? 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#discussion_r499666429



##########
File path: apisix/init.lua
##########
@@ -82,6 +75,14 @@ end
 
 
 function _M.http_init_worker()
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       why do we need to remove this line?
   It is necessary for debugging.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#discussion_r501388573



##########
File path: apisix/init.lua
##########
@@ -82,6 +75,14 @@ end
 
 
 function _M.http_init_worker()
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       it not for the user, the developer need it for testing.
   
   here is the code link of the test case: https://github.com/apache/apisix/pull/2357/files#diff-a46ca98b2569d841ad4742ef90427a1fR39
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] spacewander commented on a change in pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#discussion_r502190093



##########
File path: apisix/init.lua
##########
@@ -82,6 +75,14 @@ end
 
 
 function _M.http_init_worker()
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       @membphis 
   I think we should add a comment for it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis commented on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703362701


   @zlm0125 welcome to review and make a test with this PR. I think it should work fine for your case


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis merged pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2357:
URL: https://github.com/apache/apisix/pull/2357


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis commented on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703379020


   > why only `init` phase is not enough?
   
   `init` works in `master` process if the [lua_code_cache](https://github.com/openresty/lua-nginx-module#lua_code_cache) is `on`.
   
   We want to set different `seed` for different work processes, so we should use `init_worker` phase.
   
   we can delete those code https://github.com/apache/apisix/pull/2357/files#diff-d982d52466e7c93c7b604358339b2a29R85-R90 . 
   And run the test case, then we will get the same random number which is wrong.
   
   ![image](https://user-images.githubusercontent.com/6814606/95037817-dc27fc80-06fe-11eb-8d74-160d68ebf022.png)
   
   Here is the right one:
   
   ![image](https://user-images.githubusercontent.com/6814606/95037874-1db8a780-06ff-11eb-8b5c-e79016eb441b.png)
   
   
   ![image](https://user-images.githubusercontent.com/6814606/95037656-43917c80-06fe-11eb-8444-f00a930cac9b.png)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] moonming commented on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703379891


   > related PR: #2306
   
   I don't understand what it has to do with this PR


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] moonming edited a comment on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
moonming edited a comment on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703379891


   > related PR: #2306
   
   I don't understand what it has to do with this PR.
   https://github.com/apache/apisix/pull/2306 did not show any details.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] moonming commented on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703380224


   And we should remove random in `init` phase if add random in `init_worker` phase.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] moonming commented on a change in pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#discussion_r499571920



##########
File path: apisix/init.lua
##########
@@ -82,6 +75,14 @@ end
 
 
 function _M.http_init_worker()
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       should be removed

##########
File path: apisix/init.lua
##########
@@ -759,6 +760,14 @@ end
 
 function _M.stream_init_worker()
     core.log.info("enter stream_init_worker")
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random stream test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       ditto




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis commented on pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#issuecomment-703591388


   > And we should remove random in `init` phase if add random in `init_worker` phase.
   
   you are right. let me do this later.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] spacewander commented on a change in pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2357:
URL: https://github.com/apache/apisix/pull/2357#discussion_r502190093



##########
File path: apisix/init.lua
##########
@@ -82,6 +75,14 @@ end
 
 
 function _M.http_init_worker()
+    local seed, err = core.utils.get_seed_from_urandom()
+    if not seed then
+        core.log.warn('failed to get seed from urandom: ', err)
+        seed = ngx_now() * 1000 + ngx.worker.pid()
+    end
+    math.randomseed(seed)
+    core.log.info("random test in [1, 10000]: ", math.random(1, 1000000))

Review comment:
       @membphis 
   I think we should add a comment for it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] membphis merged pull request #2357: bugfix: set random seed for each worker process at `init_worker` phase, only `init` phase is not enough.

Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2357:
URL: https://github.com/apache/apisix/pull/2357


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org