Make variable query only support synchronous operation.

[hongzhidao]

Initially, variable query was designed to accomodate both synchronous and asynchronous operations. However, upon consideration of actual requirements, we recognized that asynchronous support was not needed. This refactoring is intended to simplify the usage of variable queries and prepare for the subsequent feature of variable updating.

[ac000]

Couple of quick questions.

What variables are we talking about here?

Is the afore mentioned discussion available anywhere?

[hongzhidao]

What variables are we talking about here?

Variable-related APIs, correspond to nxt_tstr_{api}.

Is the afore mentioned discussion available anywhere?

I'm afraid no discussion was mentioned.

[ac000]

│ What variables are we talking about here?

Variable-related APIs, correspond to nxt_tstr_{api}.

njs variables?

│ Is the afore mentioned discussion available anywhere?

I'm afraid no discussion was mentioned.

I thought the below might have been discussed somewhere...

upon consideration of actual requirements, we recognized that asynchronous support was not needed.

[hongzhidao]

njs variables?

It means Unit variables, for example, $host. This has nothing to do with njs here.

I thought the below might have been discussed somewhere...

We did consider supporting certain variables, such as $db_query_some, so that's why the API has callbacks ready and error handlers that are called after the async operations. But no such requirements are asked.

[ac000]

It means Unit variables, for example, $host . This has nothing to do with njs here.

Perhaps that should be made clear in the commit message then...

[hongzhidao]

It means Unit variables, for example, $host . This has nothing to do with njs here.

Perhaps that should be made clear in the commit message then...

Added, thanks.

[ac000]

I wonder if

4d515d54 Var: Removed unused functions and structure fields

and

2cb6b1bf Var: Removed unused functions

should be combined?

[hongzhidao]

I wonder if

4d515d54 Var: Removed unused functions and structure fields

and

2cb6b1bf Var: Removed unused functions

should be combined?

Good idea.

[ac000]

HI @hongzhidao

The only thing you might want to do to be consistent with recent commits is to lowercase the Var's and HTTPs in the commit subjects...

[hongzhidao]

HI @hongzhidao

The only thing you might want to do to be consistent with recent commits is to lowercase the Var's and HTTPs in the commit subjects...

Good reminder, thanks. Btw, I'll wait until variable updating feature or demo is ready before submitting these patches.

[callahad]

Status: The team needs to discuss if we merge this now or wait until variable updating is done.

As I understand it:

1. This PR is intended to ease the development of a new feature (updating Unit variables).
2. That feature is not yet ready.
3. We can't be certain that this refactor actually achieves its goals with its current design until that feature is developed.

Merging now avoids the risk of merge conflicts and burden of re-review, while waiting ensures that these patches are fully correct, necessary, and sufficient.

[callahad]

The team's decision is to defer to @hongzhidao :)

If you think we should merge it as-is, please do so.

Otherwise, please convert it back to a draft until it's ready for re-review and merging. There's a button at the top right under Reviewers:

[ac000]

Ah, while this is awaiting merging (or not), we should change the commit subjects to be in "imperative mood", e.g

var: Restrict nxt_tstr_query() to only support synchronous operation => no change (already imperative mood) http: Refactored return action => http: Refactor return action http: Refactored route pass query => http: Refactor route pass query http: Refactored static action => http: Refactor static action http: Refactored access log write => http: Refactor access log write var: Removed unused functions and structure fields => var: Remove unused functions and structure fields http: Added "if" option to match object => http: Add "if" option to match object

[hongzhidao]

Ah, while this is awaiting merging (or not), we should change the commit subjects to be in "imperative mood", e.g

var: Restrict nxt_tstr_query() to only support synchronous operation => no change (already imperative mood) http: Refactored return action => http: Refactor return action http: Refactored route pass query => http: Refactor route pass query http: Refactored static action => http: Refactor static action http: Refactored access log write => http: Refactor access log write var: Removed unused functions and structure fields => var: Remove unused functions and structure fields http: Added "if" option to match object => http: Add "if" option to match object

Ok. And I'm going to merge the prepared patches after I test the "if" option, then I'll create a new PR for it.

[ac000]

No need to make a new PR. You can un-draft this one!

See the Ready for review button? (just below the checks...)

I've had a look at the latest patch, I'm assuming the rest didn't change and it was just rebased against master... You can always re-request a persons review if things changed enough to warrant re-reviewing...

[hongzhidao]

Hi all, Welcome to review. Next, we need to add tests and I'm trying to refactor out the duplicate code between match "if" and access log "if", I feel they can be reused.

[hongzhidao]

Hi @ac000, Would you mind I merge the prepared variable related patches now? I think they are ready.

[ac000]

Hi @ac000, Would you mind I merge the prepared variable related patches now? I think they are ready.

Which patches exactly? These still have at least the typo in one of the subjects...

[hongzhidao]

Which patches exactly?

Except for those patches on the last one. I mean the refactoring patches.

These still have at least the typo in one of the subjects...

Fixed, thanks.

[ac000]

Could you push just what you intend to merge for clarity?

[hongzhidao]

I'm going to push the following commits first if you think their review is done.

var: Restrict nxt_tstr_query() to only support synchronous 
http: Refactor return action
http: Refactor route pass query
http: Refactor static action
http: Refactor access log write
var: Remove unused functions and structure fields

Then I'll post a new refactoring patch in front of the patch of "if" option, which I'll introduce nxt_tstr_cond_t for reuse.

[ac000]

Ah, right I see now, OK then...

[hongzhidao]

Well, it looks like I can't post part of the PR patches. Will post them together after the review is done.

[ac000]

Yeah, it can be done, but can be a bit fiddly...

I'll remove my approval while I review the new stuff...

[hongzhidao]

Hi @ac000,

I'm fine with both of the two styles when we do some features like this: a. split into two patches.

1. Introduce a prepared refactoring patch.
2. Replace existing code with corresponding structures/functions, etc.

b. just finish it once.

Refactor existing code.

It depends on the code size and whether there is too much crossed code in some functions or not. If it's small and has no crossed code, I prefer the second style.

Here this PR is this style. For example:

--- a/src/nxt_router_access_log.c
+++ b/src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }

But I'm fine if developers like to split it into two commits when I review it. So let me keep it as it is.

Although I'm curious for the reason to move nxt_http_request_access_log()?

The function has only about 4 lines of code after replacing its code with nxt_http_cond_value(). I think it's time to get rid of it.

[ac000]

Hi @ac000,

I'm fine with both of the two styles when we do some features like this: a. split into two patches.

1. Introduce a prepared refactoring patch.
2. Replace existing code with corresponding structures/functions, etc.

b. just finish it once.

Refactor existing code.

But this is more than just a refactoring... which makes it hard to review... we have multiple different types of changes all muddled up... whereas each commit should make one logical change...

It depends on the code size and whether there is too much crossed code in some functions or not. If it's small and has no crossed code, I prefer the second style.

Here this PR is this style. For example:

--- a/src/nxt_router_access_log.c
+++ b/src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);

-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }

But I'm fine if developers like to split it into two commits when I review it. So let me keep it as it is.

Although I'm curious for the reason to move nxt_http_request_access_log()?

The function has only about 4 lines of code after replacing its code with nxt_http_cond_value(). I think it's time to get rid of it.

Hmm, I'm talking about nxt_http_request_access_log() which has been moved, renamed & changed all in one go!

[hongzhidao]

I’m afraid splitting into smaller patches doesn’t make review easier. It will be like this:

1. Introduce nxt_tstr_cond_t stuff.
2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I feel the confusion is because of the remove of nxt_http_request_access_log, right?

Btw, moving it should happen finally.

[ac000]

I’m afraid splitting into smaller patches doesn’t make review easier. It will be like this:

1. Introduce nxt_tstr_cond_t stuff.

2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I think it would. The patches would be smaller and more concise...

I feel the confusion is because of the remove of nxt_http_request_access_log, right?

It certainly muddies things...

[hongzhidao]

1. Introduce nxt_tstr_cond_t stuff.
2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I think it would. The patches would be smaller and more concise...

It's the topic of the style I mentioned above, I'm fine with both of the styles.

It certainly muddies things...

So we are talking about the change of nxt_http_request_access_log(). I didn't delete it at first, it's like this:

static nxt_int_t
nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
    nxt_router_conf_t *rtcf)
{
   if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
        access_log->handler(task, r, access_log, rtcf->log_format);
        return NXT_OK;
    }

    return NXT_DECLIEND;
}

So removing it should happen at last. I feel it's ok to do it once.

[ac000]

1. Introduce nxt_tstr_cond_t stuff.
2. Rework nxt_http_request_access_log() with nxt_tstr_cond_t.

I think it would. The patches would be smaller and more concise...

It's the topic of the style I mentioned above, I'm fine with both of the styles.

I think we'll have to agree to disagree...

It certainly muddies things...

So we are talking about the change of nxt_http_request_access_log(). I didn't delete it at first, it's like this:

static nxt_int_t
nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
    nxt_router_conf_t *rtcf)
{
   if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
        access_log->handler(task, r, access_log, rtcf->log_format);
        return NXT_OK;
    }

    return NXT_DECLIEND;
}

So removing it should happen at last. I feel it's ok to do it once.

OK, so staring at it some more, I think the issue is that nxt_http_request_access_log() and nxt_http_cond_value() look very similar.

If you had changed nxt_http_request_access_log() in place rather than, I mean it looks like it was moved, but OK, adding the new function in a different location, things would look very different.

Hmm, let me show you what I mean. Refactor out nxt_tstr_cond_t from access log. (we should loose the trailing . btw..) would now look like

diff --git ./src/nxt_http.h ./src/nxt_http.h
index fe5e72a8..5369c8e1 100644
--- ./src/nxt_http.h
+++ ./src/nxt_http.h
@@ -441,6 +441,9 @@ void nxt_h1p_complete_buffers(nxt_task_t *task, nxt_h1proto_t *h1p,
     nxt_bool_t all);
 nxt_msec_t nxt_h1p_conn_request_timer_value(nxt_conn_t *c, uintptr_t data);
 
+int nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond);
+
 extern const nxt_conn_state_t  nxt_h1p_idle_close_state;
 
 #endif  /* _NXT_HTTP_H_INCLUDED_ */
diff --git ./src/nxt_http_request.c ./src/nxt_http_request.c
index ccd2b141..b142a052 100644
--- ./src/nxt_http_request.c
+++ ./src/nxt_http_request.c
@@ -24,8 +24,6 @@ static void nxt_http_request_proto_info(nxt_task_t *task,
 static void nxt_http_request_mem_buf_completion(nxt_task_t *task, void *obj,
     void *data);
 static void nxt_http_request_done(nxt_task_t *task, void *obj, void *data);
-static nxt_int_t nxt_http_request_access_log(nxt_task_t *task,
-    nxt_http_request_t *r, nxt_router_conf_t *rtcf);
 
 static u_char *nxt_http_date_cache_handler(u_char *buf, nxt_realtime_t *now,
     struct tm *tm, size_t size, const char *format);
@@ -861,12 +859,12 @@ nxt_http_request_error_handler(nxt_task_t *task, void *obj, void *data)
 void
 nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 {
-    nxt_int_t                ret;
     nxt_http_proto_t         proto;
     nxt_router_conf_t        *rtcf;
     nxt_http_request_t       *r;
     nxt_http_protocol_t      protocol;
     nxt_socket_conf_joint_t  *conf;
+    nxt_router_access_log_t  *access_log;
 
     r = obj;
     proto.any = data;
@@ -878,8 +876,10 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
         r->logged = 1;
 
         if (rtcf->access_log != NULL) {
-            ret = nxt_http_request_access_log(task, r, rtcf);
-            if (ret == NXT_OK) {
+            access_log = rtcf->access_log;
+
+            if (nxt_http_cond_value(task, r, &rtcf->log_cond)) {
+                access_log->handler(task, r, access_log, rtcf->log_format);
                 return;
             }
         }
@@ -911,34 +911,34 @@ nxt_http_request_close_handler(nxt_task_t *task, void *obj, void *data)
 }
 
 
-static nxt_int_t
-nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
-    nxt_router_conf_t *rtcf)
+int
+nxt_http_cond_value(nxt_task_t *task, nxt_http_request_t *r,
+    nxt_tstr_cond_t *cond)
 {
-    nxt_int_t                ret;
-    nxt_str_t                str;
-    nxt_bool_t               expr;
-    nxt_router_access_log_t  *access_log;
+    nxt_int_t          ret;
+    nxt_str_t          str;
+    nxt_bool_t         expr;
+    nxt_router_conf_t  *rtcf;
 
-    access_log = rtcf->access_log;
+    rtcf = r->conf->socket_conf->router_conf;
 
     expr = 1;
 
-    if (rtcf->log_expr != NULL) {
+    if (cond->expr != NULL) {
 
-        if (nxt_tstr_is_const(rtcf->log_expr)) {
-            nxt_tstr_str(rtcf->log_expr, &str);
+        if (nxt_tstr_is_const(cond->expr)) {
+            nxt_tstr_str(cond->expr, &str);
 
         } else {
             ret = nxt_tstr_query_init(&r->tstr_query, rtcf->tstr_state,
                                       &r->tstr_cache, r, r->mem_pool);
             if (nxt_slow_path(ret != NXT_OK)) {
-                return NXT_DECLINED;
+                return -1;
             }
 
-            ret = nxt_tstr_query(task, r->tstr_query, rtcf->log_expr, &str);
+            ret = nxt_tstr_query(task, r->tstr_query, cond->expr, &str);
             if (nxt_slow_path(ret != NXT_OK)) {
-                return NXT_DECLINED;
+                return -1;
             }
         }
 
@@ -952,12 +952,7 @@ nxt_http_request_access_log(nxt_task_t *task, nxt_http_request_t *r,
         }
     }
 
-    if (rtcf->log_negate ^ expr) {
-        access_log->handler(task, r, access_log, rtcf->log_format);
-        return NXT_OK;
-    }
-
-    return NXT_DECLINED;
+    return cond->negate ^ expr;
 }
 
 
diff --git ./src/nxt_router.h ./src/nxt_router.h
index cfc7258c..06c6bb32 100644
--- ./src/nxt_router.h
+++ ./src/nxt_router.h
@@ -54,8 +54,7 @@ typedef struct {
 
     nxt_router_access_log_t  *access_log;
     nxt_tstr_t               *log_format;
-    nxt_tstr_t               *log_expr;
-    uint8_t                  log_negate;  /* 1 bit */
+    nxt_tstr_cond_t          log_cond;
 } nxt_router_conf_t;
 
 
diff --git ./src/nxt_router_access_log.c ./src/nxt_router_access_log.c
index cc8d5e4f..afecd0b1 100644
--- ./src/nxt_router_access_log.c
+++ ./src/nxt_router_access_log.c
@@ -143,15 +143,8 @@ nxt_router_access_log_create(nxt_task_t *task, nxt_router_conf_t *rtcf,
     if (alcf.expr != NULL) {
         nxt_conf_get_string(alcf.expr, &str);
 
-        if (str.length > 0 && str.start[0] == '!') {
-            rtcf->log_negate = 1;
-
-            str.start++;
-            str.length--;
-        }
-
-        rtcf->log_expr = nxt_tstr_compile(rtcf->tstr_state, &str, 0);
-        if (nxt_slow_path(rtcf->log_expr == NULL)) {
+        ret = nxt_tstr_cond_compile(rtcf->tstr_state, &str, &rtcf->log_cond);
+        if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
         }
     }
diff --git ./src/nxt_tstr.c ./src/nxt_tstr.c
index a6d2e7ad..50df4c47 100644
--- ./src/nxt_tstr.c
+++ ./src/nxt_tstr.c
@@ -196,6 +196,26 @@ nxt_tstr_state_release(nxt_tstr_state_t *state)
 }
 
 
+nxt_int_t
+nxt_tstr_cond_compile(nxt_tstr_state_t *state, nxt_str_t *str,
+    nxt_tstr_cond_t *cond)
+{
+    if (str->length > 0 && str->start[0] == '!') {
+        cond->negate = 1;
+
+        str->start++;
+        str->length--;
+    }
+
+    cond->expr = nxt_tstr_compile(state, str, 0);
+    if (nxt_slow_path(cond->expr == NULL)) {
+        return NXT_ERROR;
+    }
+
+    return NXT_OK;
+}
+
+
 nxt_bool_t
 nxt_tstr_is_const(nxt_tstr_t *tstr)
 {
diff --git ./src/nxt_tstr.h ./src/nxt_tstr.h
index 2aa905df..aca74e20 100644
--- ./src/nxt_tstr.h
+++ ./src/nxt_tstr.h
@@ -37,12 +37,20 @@ typedef enum {
 } nxt_tstr_flags_t;
 
 
+typedef struct {
+    nxt_tstr_t          *expr;
+    uint8_t             negate;  /* 1 bit */
+} nxt_tstr_cond_t;
+
+
 nxt_tstr_state_t *nxt_tstr_state_new(nxt_mp_t *mp, nxt_bool_t test);
 nxt_tstr_t *nxt_tstr_compile(nxt_tstr_state_t *state, const nxt_str_t *str,
     nxt_tstr_flags_t flags);
 nxt_int_t nxt_tstr_test(nxt_tstr_state_t *state, nxt_str_t *str, u_char *error);
 nxt_int_t nxt_tstr_state_done(nxt_tstr_state_t *state, u_char *error);
 void nxt_tstr_state_release(nxt_tstr_state_t *state);
+nxt_int_t nxt_tstr_cond_compile(nxt_tstr_state_t *state, nxt_str_t *str,
+    nxt_tstr_cond_t *cond);
 
 nxt_bool_t nxt_tstr_is_const(nxt_tstr_t *tstr);
 void nxt_tstr_str(nxt_tstr_t *tstr, nxt_str_t *str);

The diffstat goes from

 src/nxt_http.h              |   3 +++
 src/nxt_http_request.c      | 105 ++++++++++++++++++++++++++++++++++++----------------------------------------
 src/nxt_router.h            |   3 +--
 src/nxt_router_access_log.c |  11 ++------
 src/nxt_tstr.c              |  20 +++++++++++++++
 src/nxt_tstr.h              |   8 ++++++
 6 files changed, 84 insertions(+), 66 deletions(-)

to

 src/nxt_http.h              |  3 +++
 src/nxt_http_request.c      | 45 ++++++++++++++++++++-------------------------
 src/nxt_router.h            |  3 +--
 src/nxt_router_access_log.c | 11 ++---------
 src/nxt_tstr.c              | 20 ++++++++++++++++++++
 src/nxt_tstr.h              |  8 ++++++++
 6 files changed, 54 insertions(+), 36 deletions(-)

Whether you think it looks like a simpler patch (we no longer have the large chuck of code removed and then another large chunk of added code) or not you can't argue with numbers...