Skip to content

fix http stream not being closed fully#3243

Open
danilp-id wants to merge 2 commits intomthom:masterfrom
danilp-id:master
Open

fix http stream not being closed fully#3243
danilp-id wants to merge 2 commits intomthom:masterfrom
danilp-id:master

Conversation

@danilp-id
Copy link

fixes #3239

Copy link
Contributor

@Skgland Skgland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that explains why that function was suddenly unused.
I even left a todo that should now be removed

impl HttpWriteStream {
// TODO why is this suddenly dead code and should it be used somewhere?
// Should this be impl Drop for HttpWriteStream?
#[allow(dead_code)]
fn drop(&mut self) {

But now I think this will still leak the headers and buffer when the slab is dropped normally so I think as suggested in the comment the inherent drop function should be moved to a Drop impl so that the content is dropped no matter how the HttpStreamWriter is dropped.

@danilp-id
Copy link
Author

fair enough

@danilp-id
Copy link
Author

@Skgland I'd also like to write a test for this, but it's a bit tricky with the way things are right now.

This is roughly what I am thinking:

test :-
    '$http_listen'("0.0.0.0:4321", HttpListener, "", "", 32768),
    accept(HttpListener),
    accept(HttpListener).

accept(HttpListener) :-
    '$http_accept'(HttpListener, _, _, _, _, _, ResponseHandle),
    '$http_answer'(ResponseHandle, 404, [], ResponseStream),
    close(ResponseStream).

The problem is that '$http_accept' is blocking. In rust code there's already a loop over timeout logic https://github.com/mthom/scryer-prolog/blob/master/src/machine/system_calls.rs#L4718 and I propose we move it to prolog. Besides making it easier to test, this would also allow more use cases like listening and accepting requests from multiple ports at the same time.

Ideally though I'd like to have something akin to select system call that would allow to code all IO logic in prolog directly, like this:

run :-
    '$http_listen'("0.0.0.0:4321", H1, "", "", 32768),
    '$http_listen'("0.0.0.0:4322", H2, "", "", 32768),
    '$http_listen'("0.0.0.0:4323", H3, "", "", 32768),
    '$http_listen'("0.0.0.0:4324", H4, "", "", 32768),
    select([H1, H2, H3, H4], H),
    % ... do some logic based on which H_ was actually invoked

@Skgland
Copy link
Contributor

Skgland commented Jan 29, 2026

@Skgland I'd also like to write a test for this, but it's a bit tricky with the way things are right now.

This is roughly what I am thinking:

test :-
    '$http_listen'("0.0.0.0:4321", HttpListener, "", "", 32768),
    accept(HttpListener),
    accept(HttpListener).

accept(HttpListener) :-
    '$http_accept'(HttpListener, _, _, _, _, _, ResponseHandle),
    '$http_answer'(ResponseHandle, 404, [], ResponseStream),
    close(ResponseStream).

The problem is that '$http_accept' is blocking. In rust code there's already a loop over timeout logic https://github.com/mthom/scryer-prolog/blob/master/src/machine/system_calls.rs#L4718 and I propose we move it to prolog. Besides making it easier to test, this would also allow more use cases like listening and accepting requests from multiple ports at the same time.

I suppose the problem with $http_accept being blocking is that it prevents you from making a connection to the socket at the same time, right? In that case would it be an option to perform the other halve either from rust i.e. spawn a second thread during test setup or from a second prolog process spawned using library(process) ?

Note

For integration tests the path to the scryer-prolog executable is available at compile time in the environment variable CARGO_BIN_EXE_scryer-prolog1

Moving this to prolog seams like a larger change that should probably be a separate PR.

Ideally though I'd like to have something akin to select system call that would allow to code all IO logic in prolog directly, like this:

run :-
    '$http_listen'("0.0.0.0:4321", H1, "", "", 32768),
    '$http_listen'("0.0.0.0:4322", H2, "", "", 32768),
    '$http_listen'("0.0.0.0:4323", H3, "", "", 32768),
    '$http_listen'("0.0.0.0:4324", H4, "", "", 32768),
    select([H1, H2, H3, H4], H),
    % ... do some logic based on which H_ was actually invoked

There were some discussions regarding parallelism/async that appear related on first glance and as such might be of interest e.g. #3031 #3034 #3035 #3047.

Footnotes

  1. https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates

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

Successfully merging this pull request may close these issues.

library(http/http_server) broken since 0.10.0 (works up to 0.9.4)

2 participants