Send sessionExpired as early as possible.

[bogdanudrescu]

Fixes #4071

Send sessionExpired as early as possible and ignore any further actions triggered by later messages regarding the invalid session. Write async together with sessionExpired.

---

This change is 

[denis-anisimov]

Reviewed 11 of 11 files at r1.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

---

flow-client/src/main/java/com/vaadin/client/communication/DefaultConnectionStateHandler.java, line 302 at r1 (raw file):

// NOTE: Need more info from Leif.

Is it still valid ?

---

flow-client/src/main/java/com/vaadin/client/communication/DefaultConnectionStateHandler.java, line 479 at r1 (raw file):

// NOTE: Need more info from Leif.

Is it still valid ?

---

flow-server/src/test/java/com/vaadin/flow/server/communication/MetadataWriterTest.java, line 117 at r1 (raw file):

writeSessionExpiredTag

There should be some checks for the case when state doesn't produce sessionExpired.

[bogdanudrescu]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

---

flow-server/src/test/java/com/vaadin/flow/server/communication/MetadataWriterTest.java, line 117 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

writeSessionExpiredTag

There should be some checks for the case when state doesn't produce sessionExpired.

@denis-anisimov Do you think the above tests (old ones which doesn't produce sessionExpired) are not enough? (of course we may add one more)

[denis-anisimov]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

---

flow-server/src/test/java/com/vaadin/flow/server/communication/MetadataWriterTest.java, line 117 at r1 (raw file):

Previously, bogdanudrescu (Bogdan Udrescu) wrote…

@denis-anisimov Do you think the above tests (old ones which doesn't produce sessionExpired) are not enough? (of course we may add one more)

Not enough.
This is just one specific case.

The code is:

        VaadinSessionState state = ui.getSession().getState();
        if ((state != null && state.compareTo(VaadinSessionState.CLOSING) >= 0)
                || ui.isClosing()) {
            meta.put(JsonConstants.META_SESSION_EXPIRED, true);
        }

So

• ui.isClosing should produce the same result
• state which is "greater" than CLOSING should produce the same result
• state which is less (or null) and ui is not closing should not produce this result.

3 checks are missing.

[bogdanudrescu]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

---

flow-server/src/test/java/com/vaadin/flow/server/communication/MetadataWriterTest.java, line 117 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Not enough.
This is just one specific case.

The code is:

        VaadinSessionState state = ui.getSession().getState();
        if ((state != null && state.compareTo(VaadinSessionState.CLOSING) >= 0)
                || ui.isClosing()) {
            meta.put(JsonConstants.META_SESSION_EXPIRED, true);
        }

So

• ui.isClosing should produce the same result
• state which is "greater" than CLOSING should produce the same result
• state which is less (or null) and ui is not closing should not produce this result.

3 checks are missing.

Done.

[denis-anisimov]

Reviewed 5 of 6 files at r2.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

[denis-anisimov]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

a discussion (no related file):
The PR in its current state (r2) is unacceptable.
Validation fails on RouterSessionExpirationIT.navigationAfterSessionExpired test.
It happens when navigation is done to the ViewWhichInvalidatesSession. The latter view invalidates the session.

Without this PR navigation works fine with No session message which is printed by Layout class.
With this PR navigation happens over and over again infinitely.

---

[denis-anisimov]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

The PR in its current state (r2) is unacceptable.
Validation fails on RouterSessionExpirationIT.navigationAfterSessionExpired test.
It happens when navigation is done to the ViewWhichInvalidatesSession. The latter view invalidates the session.

Without this PR navigation works fine with No session message which is printed by Layout class.
With this PR navigation happens over and over again infinitely.

The previous behavior has been caused by the fact that HTTP session invalidation didn't affect anyhow the current communication.
This PR sends session expiration message right away which causes the page reload. And the test page invalidates the session in before navigation event handling. It again reloads the page and that happens infinitely.

So the test view has a logical issue which needs to be fixed. But there is one mistake in the MetadaWriter which sends session expiration message if UI is closing.
UI closing has nothing to do with session expiration: one ui may be closing but session may be alive and there can be other active UIs.

I will fix all those things.

---

[bogdanudrescu]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @bogdanudrescu, @denis-anisimov, and @Legioth)

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

The previous behavior has been caused by the fact that HTTP session invalidation didn't affect anyhow the current communication.
This PR sends session expiration message right away which causes the page reload. And the test page invalidates the session in before navigation event handling. It again reloads the page and that happens infinitely.

So the test view has a logical issue which needs to be fixed. But there is one mistake in the MetadaWriter which sends session expiration message if UI is closing.
UI closing has nothing to do with session expiration: one ui may be closing but session may be alive and there can be other active UIs.

I will fix all those things.

Regarding ui.isClosing I found this note in VaadinService.createUINotFoundJSON by @Artur-

        // Session Expired is technically not really the correct thing as
        // the session exists but the requested UI does not. Still we want
        // to handle it the same way on the client side.

A closed UI and a session expired are handled internally in the same way at the moment. Maybe @Artur- could bring some more light into this matter.

---

[bogdanudrescu]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @bogdanudrescu, @denis-anisimov, and @Legioth)

---

flow-client/src/main/java/com/vaadin/client/communication/DefaultConnectionStateHandler.java, line 302 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

// NOTE: Need more info from Leif.

Is it still valid ?

This change originally came from @Legioth. Maybe he can say more about it. While we were discussing we came to the point that the endRequest exception is there to bring forth invalid states of the client, however by sending sessionExpired this was no longer necessary, so either we remove this so that it may explode later when something else goes wrong, or keep it to avoid the exception (or remove the exception thrown unless it's required somewhere else).

---

flow-client/src/main/java/com/vaadin/client/communication/DefaultConnectionStateHandler.java, line 479 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

// NOTE: Need more info from Leif.

Is it still valid ?

Same as above. This change came originally from @Legioth

[denis-anisimov]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @bogdanudrescu, @denis-anisimov, and @Legioth)

a discussion (no related file):

Previously, bogdanudrescu (Bogdan Udrescu) wrote…

Regarding ui.isClosing I found this note in VaadinService.createUINotFoundJSON by @Artur-

        // Session Expired is technically not really the correct thing as
        // the session exists but the requested UI does not. Still we want
        // to handle it the same way on the client side.

A closed UI and a session expired are handled internally in the same way at the moment. Maybe @Artur- could bring some more light into this matter.

I'm not sure VaadinService.createUINotFoundJSON should be addressed in this PR.
At least I want to see a usecase for this.

And ui.isClosing() and VaadinService.findUI() == null are not the same things .
There is already UI instance in createMetadata method and it's supposed to be not null (otherwise your code throws NPE).
So I see a ui.isClosing() check semantically wrong here.

But thanks for the info.

---

[mehdi-vaadin]

Reviewed 1 of 11 files at r1.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Artur-, @bogdanudrescu, @denis-anisimov, @Legioth, and @vaadin-bot)

---

flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java, line 18 at r3 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Remove this unused import 'java.util.Objects'.

This can be done.

---

flow-server/src/test/java/com/vaadin/flow/server/communication/CommunicationUtil.java, line 69 at r3 (raw file):

throws IOException

This is unnecessary and can be removed. Non-blocking

---

flow-server/src/test/java/com/vaadin/flow/server/communication/CommunicationUtil.java, line 76 at r3 (raw file):

contentArg.getAllValues().stream().collect(Collectors.joining())

String.join can be used here instead. Non-blocking

[denis-anisimov]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, @Legioth, and @vaadin-bot)

---

flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java, line 18 at r3 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

This can be done.

Done

---

flow-server/src/test/java/com/vaadin/flow/server/communication/CommunicationUtil.java, line 69 at r3 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

throws IOException

This is unnecessary and can be removed. Non-blocking

Done

---

flow-server/src/test/java/com/vaadin/flow/server/communication/CommunicationUtil.java, line 76 at r3 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

contentArg.getAllValues().stream().collect(Collectors.joining())

String.join can be used here instead. Non-blocking

Done

[mehdi-vaadin]

I still get this error after logout:

[Atmosphere-Shared-0] ERROR com.vaadin.flow.server.communication.PushHandler - Could not get resource. This should never happen.

It happens because AtmosphereResourceImpl.event is destroyed and therefore event.getResource() is null. See:
https://github.com/vaadin/flow/blob/bu/session-expired/flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java#L293

Logout works well though. Should we fix this as part of this PR?

Reviewed 6 of 8 files at r3, 3 of 3 files at r4.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @Artur-, @Legioth, and @vaadin-bot)

[denis-anisimov]

Do you mean the IT test in this PR ?
I've not noticed that.

So it happens because the session has been already closed and code in this method is not needed anymore as I understand.

So yes, it should be fixed as a part of this PR.

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @Artur-, @Legioth, and @vaadin-bot)

[mehdi-vaadin]

Yes, I meant the IT. So, https://github.com/vaadin/flow/blob/bu/session-expired/flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java#L293 should be fixed.

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @Artur-, @Legioth, and @vaadin-bot)

[denis-anisimov]

Alright, I will do.

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @Artur-, @Legioth, and @vaadin-bot)

[denis-anisimov]

Done

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, @Legioth, and @vaadin-bot)

[denis-anisimov]

Dismissed @vaadin-bot from a discussion.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained (waiting on @Artur-, @denis-anisimov, and @Legioth)

[mehdi-vaadin]

Reviewed 2 of 11 files at r1, 2 of 6 files at r2, 1 of 1 files at r5.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained (waiting on @Artur- and @Legioth)