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

[JENKINS-72113] Relax serialization of BoundObjectTable.Table #493

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Oct 3, 2023

JENKINS-72113

Testing done

Submitter checklist

Preview Give feedback

@Vlatombe Vlatombe requested review from jglick and daniel-beck October 3, 2023 10:57
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Noting that while this will skip some (most?) bound objects, possibly resulting in the current page being broken after session deserialization (requiring reload), that's a net improvement over not being able to serialize the session at all (and being logged out, having to log in again all over) in a suitably set up environment.

@jglick
Copy link
Member

jglick commented Oct 5, 2023

Right, this brings Stapler closer to compliance with the spec because at least other parts of the session, most crucially the login state, can be distributed.

To expand: in a deserialized session,

if (v==null) {
if (logging) LOGGER.warning(toString() + " had binding for " + id + " but it got garbage collected");
entries.remove(id); // reference is already garbage collected.
}
might be triggered. The impact would be that /$stapler/bound/… requests would give a 404 from
public Object getDynamic(String id) {
return resolve(id);
}
so any JavaScript running on the page (such as from a l:progressiveRendering) would stop with an error and the user would need to reload the page. (Similar to what already happens if your session expires while the page is open, or you log out in another tab, or the controller restarts, etc.)

This could be improved in the future to

  • st:bind a Serializable object
  • implement WithWellKnownURL
  • use other mechanisms for dynamic page content (other JS frameworks, REST API calls)

on a case-by-case basis; certain usages of st:bind (such as ProgressiveRendering) do not seem to lend themselves to distributed state without an incompatible rewrite.

In a standard Winstone setup, sessions are never serialized so this patch would have no effect.

@timja
Copy link
Member

timja commented Oct 5, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants