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.

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

Fix Server Side Event Interrupted while the page was loading error message #2005

Conversation

gungun974
Copy link

@gungun974 gungun974 commented Nov 18, 2023

Description

I love HTMX, it's for me the return of the good old way of think and I'm so glad to be here.
But nothing is perfect, there is an annoying bug in Firefox with the Server Side Event Extension.

There is an issue with the way the EventSource is closed on Firefox. When you have a currently connected EventSource and you refresh the page without closing it, you got those errors :

The connection to http://localhost:5000/stream was interrupted while the page was loading. htmx.org:1:1349
error { target: EventSource, isTrusted: true, srcElement: EventSource, currentTarget: EventSource, eventPhase: 2, bubbles: false, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }
htmx.org:1:25781

With some search on the internet I found this is an old bug from Firefox and this is not fixed yet. https://bugzilla.mozilla.org/show_bug.cgi?id=833462

By chance a workaround was found, if the JavaScript close the EventSource before the page unload. There is no more error on the page unload.

Note : There is a bug very similar on Chromium where the is also a similar error message except it appear for a fraction of time before vanish to the oblivion ! Closing the EventSource solve this too.

Testing

I know it's bad but I didn't manage a way to test my change. What my PR do is I close the EventSource on the windows beforeunload, this event make me force in my test to close the window. But if I close the window my test stop...
I don't know how I can still use beforeunload but simulate a window close in my test.
So I no my PR is not conform yet to the standard of PR but if someone can suggest a way to test beforeunload, I will update this PR to include a test to verify the EventSource is closed when the window close.

Replicate

I didn't create an Issue since It's written in the review guideline "Correspondingly, it is fine to directly PR bugfixes for behavior that htmx already guarantees", but I will still write here a small example to get those error I show in Firefox.

You need a server with SSE. (here an example in python with Flask)

from flask import Flask, render_template, Response
import time

app = Flask(__name__)

def generate_sse():
    count = 0
    while True:
        time.sleep(0.001)
        count += 1
        yield f"data: {count}\n\n"

@app.route('/stream')
def stream():
    return Response(generate_sse(), mimetype='text/event-stream')

@app.route('/')
def home():
    return render_template('index.html')

if __name__ == '__main__':
    app.run(debug=True)

And you need to use the HTMX SSE extension.

<!DOCTYPE html>
<html lang="fr">
<head>
    <meta charset="UTF-8">
    <title>Exemple HTMX SSE</title>
    <script src="https://unpkg.com/htmx.org"></script>
    <script src="https://unpkg.com/htmx.org/dist/ext/sse.js"></script>
</head>
<body>
    <h1>Compteur SSE avec HTMX</h1>
    <div hx-ext="sse" sse-connect="/stream" sse-swap="message">
        Waiting for data...
    </div>
</body>
</html>

When you reload the page, you got those errors in Firefox

@gungun974 gungun974 changed the base branch from master to dev November 18, 2023 07:20
@1cg
Copy link
Contributor

1cg commented Nov 19, 2023

@benpate @Renerick please review

src/ext/sse.js Outdated
@@ -135,6 +135,14 @@ This extension adds support for Server Sent Events to htmx. See /www/extensions
var source = htmx.createEventSource(sseURL);
internalData.sseEventSource = source;

// Don't forget to disconnect the EventSource on page unload
window.addEventListener("beforeunload", function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to remove this event listener when SSE source is closed early, e.g. when its element is removed. I'd say, make the handler a named function and add invocation of removeEventListener("beforeunload", ... in maybeCloseSSESource

Copy link
Author

Choose a reason for hiding this comment

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

I made the commit 3b63882 like you propose before reverting it when I realize a problem. (By the way this commit can't work)

I can't create a named function for the listener since I need to call api.getInternalData(elt) with the element.

I need to create if I want to externalize to an other function an another closure that take elt and pass to my named function. That don't solve that.

After I don't know how heavy is for the browser to keep eventListeners active but in my handler. I check if the EventSource is closed or not so doing what I do like this don't create problem on disconnecting a disconnected EventSource but I undertand I prefer also myself to clean up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I see the problem, good catch. As for how to solve it, that may require a bit of a refactor, kinda like you describe. I can't figure out easy solution right now sadly, but hope to come up with something soon, or maybe someone else from the team has an idea in the meantime.

While the event listeners themselves are probably very lightweight, this particular handler creates a closure that holds elt variable, which most likely will prevent the browser from properly garbage collecting upon element removal. I haven't checked it though, so don't quote me on that.

Also, I noticed that you seem to have used spaces for indentation, but this file uses tabs, so please make sure that your fix is also indented with tabs.

Copy link
Author

@gungun974 gungun974 Nov 19, 2023

Choose a reason for hiding this comment

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

Thanks for noticed this file was using tabs. To be honest I don't have any problem to use tabs VS space, it's just I never tough about that and so my neovim was configured with space for indent.

Anyway since I place my closure "beforeunload" in the same function that create the eventSource, maybe I don't need to refetch the InternalData and I can just reuse the same source. I don't know how exactly GC work in JS but I think using the variable source up scope directly should have the same behavior and except we don't need to track an element in GC. So I do what I said here in a0680a1.

When I said "it should" this is because I don't know yet how to write a Test that check our EventSource close method is called when we close the browser. But this is an issue of another conversation I think.

@gungun974 gungun974 force-pushed the fix-sse-interrupted-while-the-page-was-loading branch from 5bfdb4a to a0680a1 Compare November 19, 2023 16:45
@alexpetros alexpetros added the extension Consideration for an extension label Dec 20, 2023
@reedspool
Copy link

Blah at chasing down an error that's not actually an issue but that happens every time you refresh in just one browser. Hope y'all figure out a nice way to solve it here soon.

Here's a hack workaround that I'm using for my project until this is fixed properly.

globalThis.addEventListener("beforeunload", () => document.querySelectorAll("[sse-connect]").forEach((elt) => elt['htmx-internal-data'].sseEventSource.close()))

@Telroshan
Copy link
Collaborator

Hey @gungun974 , following up very late on this, sorry!
As extensions have now moved to a separate repository, I'll close this PR.
If you are still interested in this change, please open a PR on the extensions repo

@Telroshan Telroshan closed this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Consideration for an extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants