jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.83k stars 1.91k forks source link

Introduce locking for core HTTP `Session` #10392

Open sbordet opened 1 year ago

sbordet commented 1 year ago

Jetty version(s) 12

Enhancement Description Currently the way to use core HTTP org.eclipse.jetty.server.Session concurrently is to use synchronized around it.

This is not virtual threads friendly, so we should have a mechanism such as:

gregw commented 1 year ago

Not that the servlet spec says that the HttpSession object is not thread safe and that applications must do their own synchronisation: https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#threading-issues It is thus likely that some/many applications also synchronize on the session object, thus if we switch to locks we not only do not mutually exclude them, but those apps will still not be virtual thread safe.

If a session is to have an internal lock, then we would need to advocate that to be a standard API and wait for all the apps that use sessions to be updated.

sbordet commented 1 year ago

Not sure I follow.

Servlet apps will lock using synchronized on a different instance, that delegates to the core Session object.

Core web apps, if they wish to lock, will have a clear API to use.

janbartel commented 1 year ago

@sbordet not sure I follow what you're advocating. None of our existing session code uses synchronized, we do our own locking.

If you're talking about apps needing to synchronize usage of HttpSession objects, then they will most likely use synchronized, rather than make their app non-portable by using some jetty-specific classes to support locking.

gregw commented 1 year ago

@sbordet as @janbartel has just pointed out to me, there is a public AutoLock lock() on ManagedSession (but not exposed in our Session interface). That is the lock that is used for internal updates of the session state (passivated, activated, id change, invalidation, etc.).

But that is not exposed to the applications. Currently the applications just synchronize on the session object itself and they do this when setting attributes to mutually exclude other requests doing so. You are right that servlets will again wrap our Session as a HttpSession, so there is an issue if we mix handler and servlet code. But ultimately applications are generally beyond our control and they are going to synchronize on session. We can change our authenticators to use the internal AutoLock, but that will not make sessions much more virtual thread safe, because applications are beyond our control.

Actually, we should change our authenticators to synchronize on Session.getAPI(), so we do use the servlet API wrappers if they are present.

I just don't see how we can pave over this deficiency of virtual threads when we are invoking application code beyond our control?

sbordet commented 1 year ago

@gregw @janbartel I am talking about core-only web applications.

public boolean handle(Request req, Response rsp, Callback cbk) {
    Session session = req.getSession(true);
    // How to make sure access to the session is thread-safe.
    // Don't want to use synchronized.
}
github-actions[bot] commented 3 weeks ago

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.