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.

Sign up for GitHub

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

Jump to bottom

qjs fetch. #864

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

qjs fetch. #864

wants to merge 1 commit into from
+3,944 −13

Conversation

hongzhidao
Copy link
Contributor

@hongzhidao hongzhidao commented Mar 17, 2025 •
edited
Loading

No description provided.

@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Here's a draft PR for QuickJS fetch support. And it passed js_fetch_object.t locally.
My next plans are:

  1. Refactor out prepared patches which are refactoring and fixing patches.
  2. Merge ngx_qjs_fetch.c into ngx_js_fetch.c and reuse code as much as possible.
  3. Support https.
  4. Test with njs-sandbox.

@hongzhidao hongzhidao closed this Mar 19, 2025
@hongzhidao hongzhidao reopened this Mar 19, 2025
@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Take a look at this.
https://github.com/nginx/njs/blob/master/nginx/ngx_js_shared_dict.c#L3042
It caused the assert error.

void JS_SetClassProto(JSContext *ctx, JSClassID class_id, JSValue obj)
{
    JSRuntime *rt = ctx->rt;
    assert(class_id < rt->class_count);
    set_value(ctx, &ctx->class_proto[class_id], obj);
}

Is it because there is no JS_NewClass() for NGX_QJS_CLASS_ID_SHARED_DICT_ERROR?

@xeioex
Copy link
Contributor

xeioex commented Mar 24, 2025 •
edited
Loading

Hi @hongzhidao,

It seems, with more classes you added you stepped upon rt->class_count limit. The class array is resized by 3/2. So, yes, we need JS_NewClass() for NGX_QJS_CLASS_ID_SHARED_DICT_ERROR.

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Mar 24, 2025 •
edited
Loading

Hi @hongzhidao,

It seems, with more classes you added you stepped upon rt->class_count limit. The class array is resized by 3/2. So, yes, it seems, we need JS_NewClass() for NGX_QJS_CLASS_ID_SHARED_DICT_ERROR.

Agree, thanks.
I'll fix it, but I need to make this PR pass with js_fetch.t first.

[update] done: #873

@hongzhidao hongzhidao mentioned this pull request Mar 24, 2025
Merged
@hongzhidao hongzhidao force-pushed the qjs-fetch branch 4 times, most recently from 89600cd to 936da2c Compare March 25, 2025 06:49
@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Does the test pass when you run it?

njs/nginx/t/js_fetch.t

Line 427 in 8708663

like(http_get('/body?getter=json&loc=json&path=b.c'),

I tested it with njs, and it failed. I debug the response. Its output is:

HTTP/1.1 403 Forbidden
Server: nginx/1.27.5
Date: Tue, 25 Mar 2025 07:22:53 GMT
Content-Type: text/html
Content-Length: 153
Connection: close

<html>
<head><title>403 Forbidden</title></head>
<body>
<center><h1>403 Forbidden</h1></center>
<hr><center>nginx/1.27.5</center>
</body>
</html>

@xeioex
Copy link
Contributor

xeioex commented Mar 26, 2025

@hongzhidao

here is my js_fetch.t output

TEST_NGINX_VERBOSE=1 TEST_NGINX_LEAVE=1 TEST_NGINX_BINARY=/path/to/objs/nginx prove -I /path/to/nginx-tests/lib/ nginx/t/js_fetch.t -v 

nginx/t/js_fetch.t .. 
# >> GET /engine HTTP/1.0\x0a
# >> Host: localhost\x0a
# >> \x0a
# << HTTP/1.1 200 OK\x0d\x0a
# << Server: nginx/1.27.4\x0d\x0a
# << Date: Wed, 26 Mar 2025 02:04:37 GMT\x0d\x0a
# << Content-Type: text/plain\x0d\x0a
# << Content-Length: 3\x0d\x0a
# << Connection: close\x0d\x0a
# << \x0d\x0a
# << njs
1..39

...

# >> GET /body?getter=text&loc=loc HTTP/1.0\x0a
# >> Host: localhost\x0a
# >> \x0a
# << HTTP/1.1 200 OK\x0d\x0a
# << Server: nginx/1.27.4\x0d\x0a
# << Date: Wed, 26 Mar 2025 02:04:37 GMT\x0d\x0a
# << Content-Type: text/plain\x0d\x0a
# << Content-Length: 7\x0d\x0a
# << Connection: close\x0d\x0a
# << \x0d\x0a
# << "GET::"
ok 2 - fetch body text
# >> GET /body?getter=json&loc=json&path=b.c HTTP/1.0\x0a
# >> Host: localhost\x0a
# >> \x0a
# << HTTP/1.1 200 OK\x0d\x0a
# << Server: nginx/1.27.4\x0d\x0a
# << Date: Wed, 26 Mar 2025 02:04:37 GMT\x0d\x0a
# << Content-Type: text/plain\x0d\x0a
# << Content-Length: 7\x0d\x0a
# << Connection: close\x0d\x0a
# << \x0d\x0a
# << "FIELD"
ok 3 - fetch body json

...

ok
All tests successful.
Files=1, Tests=39,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.17 cusr  0.05 csys =  0.24 CPU)
Result: PASS

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Mar 26, 2025 •
edited
Loading

I changed the location /json, and then it passed.

location /json { return 200 '{"a":[1,2], "b":{"c":"FIELD"}}'; }

It passed with non-root user.

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Mar 26, 2025 •
edited
Loading

Hi @xeioex,
js_fetch.t and js_fetch_objects.t passed except this case.

like(http_get('/body?getter=json&loc=loc'), qr/501/s,
    'fetch body json invalid');

But I'm not sure if it's an issue in QuickJS.
bellard/quickjs#341
Created an issue to track it.
bellard/quickjs#395

My next step is to support DNS resolve and https with fetch.

[update]: all the test cases in js_fetch.t and js_fetch_objects.t passed.

@hongzhidao hongzhidao force-pushed the qjs-fetch branch 3 times, most recently from e9e640a to cd718ff Compare March 31, 2025 06:44
@hongzhidao
Copy link
Contributor Author

Hi @xeioex,
Take a look at this log and it's related to enabled qjs.

2025/04/01 18:09:07 [notice] 1724837#0: js vm init njs: 000050E0000004A0
2025/04/01 18:09:07 [notice] 1724837#0: js vm init QuickJS: 000050E0000072E0

=================================================================
==1724838==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 434176 byte(s) in 3 object(s) allocated from:
    #0 0x7779d6cfd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x62ecc41b36ed in ngx_alloc src/os/unix/ngx_alloc.c:22

SUMMARY: AddressSanitizer: 434176 byte(s) leaked in 3 allocation(s).

I did this to reproduce it.

> ./auto/configure --with-http_ssl_module --add-module=../njs/nginx/ --with-cc-opt="-I ../quickjs -fsanitize=address -DNJS_DEBUG_MEMORY -DNGX_DEBUG_PALLOC -DNGX_DEBUG_MALLOC" --with-ld-opt="-L ../quickjs -fsanitize=address"
> ./objs/nginx
> ^ C

# config
worker_processes  1;
daemon  off;

events {
    worker_connections  1024;
}


http {
    include       mime.types;
    default_type  application/octet-stream;

    server {
        listen  80;

        location / {
            return  200;
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hongzhidao @xeioex