Remove verifyLock() from and add getLockInstance() to VaadinSession #3155

Closed
vaadin-bot opened this Issue Nov 16, 2012 · 4 comments

Comments

Projects
None yet
1 participant
@vaadin-bot
Collaborator

vaadin-bot commented Nov 16, 2012

Originally by @Artur-


Locking of the session should be done using VaadinSession.lock() and VaadinSession.unlock(). These operate on a lock instance available through getLockInstance(). To verify the lock you should use the lock instance and therefore VaadinSession.verifyLock() should be removed

@vaadin-bot

This comment has been minimized.

Show comment Hide comment
@vaadin-bot

vaadin-bot Nov 21, 2012

Collaborator

Originally by @Legioth


Reviewed by Henri Sara.

Collaborator

vaadin-bot commented Nov 21, 2012

Originally by @Legioth


Reviewed by Henri Sara.

@vaadin-bot vaadin-bot closed this Nov 21, 2012

@vaadin-bot

This comment has been minimized.

Show comment Hide comment
@vaadin-bot

vaadin-bot Nov 27, 2012

Collaborator

Originally by archie172


Replying to [#10225 Artur Signell]:

To verify the lock you should use the lock instance

Using what method?? There is no such method in the Lock interface.

I'm confused. Both #9996 and #9515 are asking for Vaadin to provide a way to verify that the current thread holds the session lock.

But there's still no way to do this using the current API, without casting to ReentrantLock, which is unsafe.

Please add VaadinSession.verifyLock() (which AFAICT never existed).

Collaborator

vaadin-bot commented Nov 27, 2012

Originally by archie172


Replying to [#10225 Artur Signell]:

To verify the lock you should use the lock instance

Using what method?? There is no such method in the Lock interface.

I'm confused. Both #9996 and #9515 are asking for Vaadin to provide a way to verify that the current thread holds the session lock.

But there's still no way to do this using the current API, without casting to ReentrantLock, which is unsafe.

Please add VaadinSession.verifyLock() (which AFAICT never existed).

@vaadin-bot

This comment has been minimized.

Show comment Hide comment
@vaadin-bot

vaadin-bot Nov 28, 2012

Collaborator

Originally by @Legioth


Replying to archie172:

I'm confused. Both #9996 and #9515 are asking for Vaadin to provide a way to verify that the current thread holds the session lock.

[[BR]]#9996 explains why Vaadin will not have any API for directly checking the lock: We have planned to improve support for distributed environments in a 7.x.0 release. Such environments might not provide a way of checking whether the lock is held by the current thread e.g. if the locking is based on a shared database or file system.

By not providing an API for verifying the lock now, we can avoid breaking that API if we come to realize that it can no longer be supported.
[[BR]][[BR]]

But there's still no way to do this using the current API, without casting to ReentrantLock, which is unsafe.

[[BR]]
#9515 will most likely be implemented with a check for instanceof ReentrantLock before casting, which is fully safe. It the instance is not a ReentrantLock, #9515 will simply not have any effect.
[[BR]][[BR]]

Please add VaadinSession.verifyLock() (which AFAICT never existed).

[[BR]]
This ticket was written based on plans for adding verifyLock() that were later scrapped because of reasons explained above. Sorry for the confusion.

Collaborator

vaadin-bot commented Nov 28, 2012

Originally by @Legioth


Replying to archie172:

I'm confused. Both #9996 and #9515 are asking for Vaadin to provide a way to verify that the current thread holds the session lock.

[[BR]]#9996 explains why Vaadin will not have any API for directly checking the lock: We have planned to improve support for distributed environments in a 7.x.0 release. Such environments might not provide a way of checking whether the lock is held by the current thread e.g. if the locking is based on a shared database or file system.

By not providing an API for verifying the lock now, we can avoid breaking that API if we come to realize that it can no longer be supported.
[[BR]][[BR]]

But there's still no way to do this using the current API, without casting to ReentrantLock, which is unsafe.

[[BR]]
#9515 will most likely be implemented with a check for instanceof ReentrantLock before casting, which is fully safe. It the instance is not a ReentrantLock, #9515 will simply not have any effect.
[[BR]][[BR]]

Please add VaadinSession.verifyLock() (which AFAICT never existed).

[[BR]]
This ticket was written based on plans for adding verifyLock() that were later scrapped because of reasons explained above. Sorry for the confusion.

@vaadin-bot

This comment has been minimized.

Show comment Hide comment
@vaadin-bot

vaadin-bot Nov 28, 2012

Collaborator

Originally by archie172


OK, thanks. I would just add two more comments:

  1. It's always possible to implement VaadinSession.verifyLock(): simply keep track of whether the current thread has invoked VaadinSession.lock() in a ThreadLocal variable.

  2. Even if you don't implement VaadinSession.verifyLock(), you could implement VaadinSession.verifyLockIfPossible() which would return this.lock instanceof ReentrantLock && ((ReentrantLock)this.lock).isHeldByCurrentThread() and document that it may be a no-op in certain distributed scenarios.

In any case, I feel this is important because race conditions caused by incorrect locking are very difficult to debug, typically result in corruption of data, and therefore need to be guarded against rigorously (hence #9515). Trust me, this is from lots of prior experience :)

Collaborator

vaadin-bot commented Nov 28, 2012

Originally by archie172


OK, thanks. I would just add two more comments:

  1. It's always possible to implement VaadinSession.verifyLock(): simply keep track of whether the current thread has invoked VaadinSession.lock() in a ThreadLocal variable.

  2. Even if you don't implement VaadinSession.verifyLock(), you could implement VaadinSession.verifyLockIfPossible() which would return this.lock instanceof ReentrantLock && ((ReentrantLock)this.lock).isHeldByCurrentThread() and document that it may be a no-op in certain distributed scenarios.

In any case, I feel this is important because race conditions caused by incorrect locking are very difficult to debug, typically result in corruption of data, and therefore need to be guarded against rigorously (hence #9515). Trust me, this is from lots of prior experience :)

@vaadin-bot vaadin-bot added the bug label Dec 9, 2016

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