Framework: Assertions in VaadinSession

[FSchliephacke]

Vaadin Framework version: 8, all versions

There is a major error in the class com.vaadin.server.VaadinSession (and possibly other classes too, if this is some kind of cargo cult programming in the Vaadin team)

Most public methods in the class use assertions to "check" if the session has the lock. This is fundamentally wrong. Assertions are the wrong tool to check preconditions in public methods. Instead the methods should indicate the missing lock by throwing an IllegalStateException or something to that effect. Unchecked exceptions indicate errors in my (the api user's) code, assertions guarantee conditions in your (the api designer's) code.

I can not stress this enough: If I use an api wrong an exception needs to be thrown. I used the VaadinSession wrong and everything worked as I expected because I did not have assertions enabled. If I need to enable your precondition checks in my runtime you could just stop checking them anyway.

To learn about using assertions correctly, read the following oracle article:
https://docs.oracle.com/javase/8/docs/technotes/guides/language/assert.html

[Legioth]

You are right that there should preferably be explicit checks and regular exception for cases where a developer might accidentally use an API in the wrong way.

Checking whether a session is locked is, however, a special case because of the performance overhead associated with doing those checks. To avoid that overhead in production, we have chosen to make those particular precondition tests use the assert keyword so that they are only run if the JVM is run with assertions enabled.

The overhead might not be a problem for methods in VaadinSession, but the same pattern is also used e.g. in AbstractClientConnector.getState(boolean). There, the check needs to recursively traverse an unknown number of getParent() references to find the UI instance and then read a volatile instance field in UI to find the session. Rather than using different conventions for lock checking in different parts of the code, we have chosen to consistently use assert.

Other precondition tests in user-facing API are still implemented in the conventional way, as can be seen in e.g. VaadinSession.setAttribute(String, Object) which explicitly checks that the name is not null.

Various parts of our documentation recommends having assertions enabled during development for exactly this reason, but I'm sure that information could be more prominently conveyed. Maybe we should also add comments in the code that clarify the situation so that someone reading the code would also have a fair chance of understanding the situation?

[FSchliephacke]

Ok, if this really is a performance issue then I guess you have thought about it and about alternatives. I am still not convinced that this is the best strategy, though.

Maybe putting a warning in the javadoc is a way to get more people to read it? I normally just skim through the Vaadin docs and then read the javadoc of methods I have never used before thoroughly.

[Legioth]

I agree. It would be great to make sure the requirement is mentioned in the JavaDocs. I also think there could be a helpful comment in the actual code right before the assert statements.

[stale[bot]]

Hello there!

We are sorry that this issue hasn't progressed lately. We are prioritizing issues by severity and the number of customers we expect are experiencing this and haven't gotten around to fix this issue yet.

There are a couple of things you could help to get things rolling on this issue (this is an automated message, so expect that some of these are already in use):

• Check if the issue is still valid for the latest version. There are dozens of duplicates in our issue tracker, so it is possible that the issue is already tackled. If it appears to be fixed, close the issue, otherwise report to the issue that it is still valid.
• Provide more details how to reproduce the issue.
• Explain why it is important to get this issue fixed and politely draw others attention to it e.g. via the forum or social media.
• Add a reduced test case about the issue, so it is easier for somebody to start working on a solution.
• Try fixing the issue yourself and create a pull request that contains the test case and/or a fix for it. Handling the pull requests is the top priority for the core team.
• If the issue is clearly a bug, use the Warranty in your Vaadin subscription to raise its priority.

Thanks again for your contributions! Even though we haven't been able to get this issue fixed, we hope you to report your findings and enhancement ideas in the future too!