vaadin / framework

Vaadin 6, 7, 8 is a Java framework for modern Java web applications.
http://vaadin.com/
Other
1.77k stars 728 forks source link

Fix providing list of changed bindings to doWriteIfValid method. #12454

Closed tdq closed 2 years ago

tdq commented 2 years ago

Thanks for submitting a pull request! Please confirm that your pull request follows our guidelines found in CONTRIBUTING and that you provide enough information so that we can review your pull request.

<Description of pull request, e.g. "Fix Grid Column not sortable with backend data and sort property">

Fixes #

Check when you have completed [ ] Valid tests for the pull request [ ] Contributing guidelines implemented

CLAassistant commented 2 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Nikolay Gorokhov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

TatuLund commented 2 years ago

Hi @tdq , could you create also an issue describing the actual problem you are trying to fix with sample test code demonstrating and replicating the problem. Also describe in this PR why you think this is the solution.

At this point of time it is hard for us to tell whether this is a valid approach or not. The PR is also failing in three existing unit tests of the Binder, which indicates that it is probably causing more problems than fixing.

tdq commented 2 years ago

Hi @TatuLund We are getting ConcurrentModificationException in our product. Here is the error log

java.util.ConcurrentModificationException: null java.base/java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:758) java.base/java.util.LinkedHashMap$LinkedKeyIterator.next(LinkedHashMap.java:780) java.base/java.lang.Iterable.forEach(Iterable.java:74) com.vaadin.data.Binder.doWriteIfValid(Binder.java:2035) com.vaadin.data.Binder.handleFieldValueChange(Binder.java:1569) com.vaadin.data.Binder$BindingImpl.handleFieldValueChange(Binder.java:1292) jdk.internal.reflect.GeneratedMethodAccessor203.invoke(Unknown Source) java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) java.base/java.lang.reflect.Method.invoke(Method.java:564) com.vaadin.event.ListenerMethod.receiveEvent(ListenerMethod.java:706) com.vaadin.event.EventRouter.fireEvent(EventRouter.java:399) com.vaadin.event.EventRouter.fireEvent(EventRouter.java:363) com.vaadin.server.AbstractClientConnector.fireEvent(AbstractClientConnector.java:1190) com.vaadin.ui.AbstractField.setValue(AbstractField.java:144) com.vaadin.ui.AbstractTextField$AbstractTextFieldServerRpcImpl.setText(AbstractTextField.java:59) jdk.internal.reflect.GeneratedMethodAccessor180.invoke(Unknown Source) java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) java.base/java.lang.reflect.Method.invoke(Method.java:564) com.vaadin.server.ServerRpcManager.applyInvocation(ServerRpcManager.java:155) com.vaadin.server.ServerRpcManager.applyInvocation(ServerRpcManager.java:116) com.vaadin.server.communication.ServerRpcHandler.handleInvocation(ServerRpcHandler.java:442) com.vaadin.server.communication.ServerRpcHandler.handleInvocations(ServerRpcHandler.java:407) com.vaadin.server.communication.ServerRpcHandler.handleRpc(ServerRpcHandler.java:274) com.vaadin.server.communication.UidlRequestHandler.synchronizedHandleRequest(UidlRequestHandler.java:90) com.vaadin.server.SynchronizedRequestHandler.handleRequest(SynchronizedRequestHandler.java:40) com.vaadin.server.VaadinService.handleRequest(VaadinService.java:1608) com.vaadin.server.VaadinServlet.service(VaadinServlet.java:448) javax.servlet.http.HttpServlet.service(HttpServlet.java:741) org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231) org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52) org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:199) org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96) org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:543) org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139) org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81) org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:688) org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:87) org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343) org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:609) org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65) org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:818) org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1623) org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130) java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630) org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) java.base/java.lang.Thread.run(Thread.java:832)

The problem is that it's not that easy to prepare example. It was faster to investigate the root of the problem and I found that changedBindings was provided into doWriteIfValid method without wrapping like in other places.

TatuLund commented 2 years ago

Your stack trace is essentialy same as this one https://github.com/vaadin/flow/issues/9217

And last week I cherry picked fix for that from Flow, https://github.com/vaadin/framework/pull/12458

Which is the correct fix, and has also unit test. We will release 8.14.2 probably this week, and it will have the fix to this issue. Thus I am closing this PR as redundant.

TatuLund commented 2 years ago

@tdq Vaadin 8.14.2 has been released.