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

Fix #2559: order of handlers: protocol first, user second #2581

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Fix #2559: order of handlers: protocol first, user second #2581

merged 2 commits into from
Jan 26, 2024

Conversation

cpq
Copy link
Member

@cpq cpq commented Jan 20, 2024

No description provided.

@cpq cpq requested a review from scaprile January 20, 2024 09:43
@scaprile
Copy link
Collaborator

scaprile commented Jan 22, 2024 •
edited
Loading

This breaks those examples where we want to process incoming messages ourselves. The protocol handler has a higher priority and deletes the data from the buffer before we get a chance to even see it.

For example:

[http-streaming-client]$ make
cc main.c mongoose.c  packed_fs.c       -W -Wall -Wextra -g -I.   -DMG_ENABLE_LINES=1 -DMG_ENABLE_PACKED_FS=1  -o example                                    
./example                    
<html><head></head><body><header>
...
</body></html>
[http-streaming-client]$ git checkout 2559 && make clean
Switched to branch '2559'
Your branch is up to date with 'origin/2559'.
rm -rf                    example                    *.o *.obj *.exe *.dSYM mbedtls
[http-streaming-client]$ make
cc main.c mongoose.c  packed_fs.c       -W -Wall -Wextra -g -I.   -DMG_ENABLE_LINES=1 -DMG_ENABLE_PACKED_FS=1  -o example                                    
./example                    
[http-streaming-client]$

No output !

mongoose/src/http.c

Lines 976 to 977 in 00ea383

static void http_cb(struct mg_connection *c, int ev, void *ev_data) {
if (ev == MG_EV_READ || ev == MG_EV_CLOSE) {

mongoose/src/http.c

Lines 1036 to 1039 in 00ea383

if (ofs > 0) mg_iobuf_del(&c->recv, 0, ofs); // Delete processed data
}
(void) ev_data;
}

@scaprile scaprile closed this Jan 22, 2024
@scaprile scaprile reopened this Jan 22, 2024
scaprile
scaprile requested changes Jan 22, 2024
View reviewed changes
Copy link
Collaborator

@scaprile scaprile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changing the order of handlers can have many unexpected and undesired effects. We'll have to probably re-write a lot of stuff.
I'd try to prevent sending EV_CLOSE when there is outstanding data, fire an EV_READ that will in this case end up being an EV_HTTP_MSG and then fire the close event. That would leave things as they are with no processing reversal.

Please see other comments
file-upload-single-post doesn't work, either, it doesn't even serve the info page.

@cpq cpq changed the title Changed the order of handlers: protocol first, user second Fix #2559: order of handlers: protocol first, user second Jan 24, 2024
@cpq
Copy link
Member Author

cpq commented Jan 24, 2024

Fixed the example.

I believe that the order of handler "proto first, user second" is semantically correct.
I has been like that all the time! It was changed, I believe, to make this streaming client work. But it was the wrong decision, IMO

@scaprile
Copy link
Collaborator

scaprile commented Jan 24, 2024 •
edited
Loading

file-upload-single-post doesn't work, either, it doesn't even serve the info page.
Those are only two I tested.
Even though the handler for that example is similar to the streaming client, it needs to detect end of file to close the file in the file system.

@cpq
Copy link
Member Author

cpq commented Jan 25, 2024

Doh... File uploads is kind of basics! We must test those automatically.
Anyhow, will look at file uploads.

@scaprile
Copy link
Collaborator

The rest of the examples tested OK.
The recently added file-transfer example will quite likely have the same issues.

@cpq
Copy link
Member Author

cpq commented Jan 25, 2024

Fixed!

The idea is to use mg_listen() instead of mg_http_listen(). This way we just get rid of the HTTP protocol handler!
I assume that to be a rare case. The uploader could run on a separate port, to avoid clashes with the rest of the UI.

@scaprile
Copy link
Collaborator

Having a separate handler because we are bypassing the default handler to shape buffering in embedded is understandable

@scaprile scaprile self-requested a review January 26, 2024 13:31
@cpq cpq merged commit b804217 into master Jan 26, 2024
133 checks passed
@scaprile scaprile deleted the 2559 branch January 31, 2024 13:14
@scaprile scaprile mentioned this pull request Feb 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scaprile scaprile scaprile approved these changes

Assignees

@cpq cpq

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cpq @scaprile