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.

Sign up for GitHub

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

CHE-1143: fixed synchronization issue during removal of several projects #1343

Merged
merged 1 commit into from
May 25, 2016
Merged

CHE-1143: fixed synchronization issue during removal of several projects #1343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...e-api-project/src/main/java/org/eclipse/che/api/vfs/impl/file/LocalVirtualFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ private boolean isVfsServicePath(Path path) {
return newArrayList(path.elements()).contains(".vfs");
}

List<VirtualFile> getChildren(LocalVirtualFile parent, VirtualFileFilter filter) throws ServerException {
synchronized List<VirtualFile> getChildren(LocalVirtualFile parent, VirtualFileFilter filter) throws ServerException {
if (parent.isFolder()) {
final List<VirtualFile> children = doGetChildren(parent, DOT_VFS_DIR_FILTER, filter);
Collections.sort(children);
Expand Down
93 changes: 74 additions & 19 deletions ...core-api-project/src/test/java/org/eclipse/che/api/project/server/ProjectServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,17 @@
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Scanner;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;

Expand Down Expand Up @@ -219,39 +225,23 @@ public void setUp() throws Exception {
FileWatcherNotificationHandler fileWatcherNotificationHandler = new DefaultFileWatcherNotificationHandler(vfsProvider);
FileTreeWatcher fileTreeWatcher = new FileTreeWatcher(root, new HashSet<>(), fileWatcherNotificationHandler);

pm = new ProjectManager(vfsProvider, null, ptRegistry, projectRegistry, phRegistry,
pm = new ProjectManager(vfsProvider, new EventService(), ptRegistry, projectRegistry, phRegistry,
importerRegistry, fileWatcherNotificationHandler, fileTreeWatcher, workspaceHolder);
pm.initWatcher();

HttpJsonRequest httpJsonRequest = mock(HttpJsonRequest.class, new SelfReturningAnswer());

//List<ProjectConfigDto> modules = new ArrayList<>();

final ProjectConfigDto testProjectConfigMock = mock(ProjectConfigDto.class);
when(testProjectConfigMock.getPath()).thenReturn("/my_project");
when(testProjectConfigMock.getName()).thenReturn("my_project");
when(testProjectConfigMock.getDescription()).thenReturn("my test project");
when(testProjectConfigMock.getType()).thenReturn("my_project_type");
// when(testProjectConfigMock.getModules()).thenReturn(modules);
when(testProjectConfigMock.getSource()).thenReturn(DtoFactory.getInstance().createDto(SourceStorageDto.class));
// when(testProjectConfigMock.findModule(anyString())).thenReturn(testProjectConfigMock);

Map<String, List<String>> attr = new HashMap<>();
for (Attribute attribute : myProjectType.getAttributes()) {
attr.put(attribute.getName(), attribute.getValue().getList());
}
when(testProjectConfigMock.getAttributes()).thenReturn(attr);

projects = new ArrayList<>();
projects.add(testProjectConfigMock);
addMockedProjectConfigDto(myProjectType, "my_project");

when(httpJsonRequestFactory.fromLink(any())).thenReturn(httpJsonRequest);
when(httpJsonRequest.request()).thenReturn(httpJsonResponse);
when(httpJsonResponse.asDto(WorkspaceDto.class)).thenReturn(usersWorkspaceMock);
when(usersWorkspaceMock.getConfig()).thenReturn(workspaceConfigMock);
when(workspaceConfigMock.getProjects()).thenReturn(projects);

pm.createProject(testProjectConfigMock, null);

// verify(httpJsonRequestFactory).fromLink(eq(DtoFactory.newDto(Link.class)
// .withHref(apiEndpoint + "/workspace/" + workspace + "/project")
// .withMethod(PUT)));
Expand Down Expand Up @@ -288,6 +278,27 @@ public Set<Object> getSingletons() {
env = org.eclipse.che.commons.env.EnvironmentContext.getCurrent();
}

private void addMockedProjectConfigDto(org.eclipse.che.api.project.server.type.ProjectTypeDef myProjectType, String projectName)
throws ForbiddenException, ServerException, NotFoundException, ConflictException {
final ProjectConfigDto testProjectConfigMock = mock(ProjectConfigDto.class);
when(testProjectConfigMock.getPath()).thenReturn("/" + projectName);
when(testProjectConfigMock.getName()).thenReturn(projectName);
when(testProjectConfigMock.getDescription()).thenReturn("my test project");
when(testProjectConfigMock.getType()).thenReturn("my_project_type");
when(testProjectConfigMock.getSource()).thenReturn(DtoFactory.getInstance().createDto(SourceStorageDto.class));
// when(testProjectConfigMock.getModules()).thenReturn(modules);
Copy link

@garagatyi garagatyi May 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why have you added commented code?

Copy link
Author

Choose a reason for hiding this comment

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

I just extracted the sources into a separate method and those comments already were there. Unfortunately there is no information about the author of that ugly code so I'm still not sure if that was left for some reason or by a mistake.

As I'm quite a new person to this very project I feel some responsibility for the legacy sources, so I think that it is yet not of my competence to remove someones comments.

Anyway if you feel confident and recommend to remove it - that should not be a problem at all ;-)

// when(testProjectConfigMock.findModule(anyString())).thenReturn(testProjectConfigMock);

Map<String, List<String>> attr = new HashMap<>();
for (Attribute attribute : myProjectType.getAttributes()) {
attr.put(attribute.getName(), attribute.getValue().getList());
}
when(testProjectConfigMock.getAttributes()).thenReturn(attr);

projects.add(testProjectConfigMock);

pm.createProject(testProjectConfigMock, null);
}

@Test
@SuppressWarnings("unchecked")
Expand Down Expand Up @@ -927,6 +938,50 @@ public void testDeleteProject() throws Exception {
pm.getProject("my_project");
}

@Test
public void testDeleteProjectsConcurrently() throws Exception {
int threadNumber = 100;
ExecutorService executor = Executors.newFixedThreadPool(threadNumber);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about non-daemon executor which is never shut down?

Copy link
Author

Choose a reason for hiding this comment

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

Well you're right, though in this very case it should not necessarily be shut down except for the reason of reclamation of its resources, but I guess I will add a call to make the code more clear.

Copy link

Choose a reason for hiding this comment

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

You can make them daemons

Copy link
Author

Choose a reason for hiding this comment

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

Sure

CountDownLatch countDownLatch = new CountDownLatch(threadNumber);
List<Future<ContainerResponse>> futures = new LinkedList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

As you know the size of the list i'd say that it is better to choose ArrayList instead of LinkedList.

Copy link
Author

Choose a reason for hiding this comment

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

I see no significant benefit of ArrayList over LinkedList here, do you?

Copy link

Choose a reason for hiding this comment

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

Array list is almost always faster

Copy link
Contributor

Choose a reason for hiding this comment

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

well it is not a significant one but yes i think i do, it is all about the heap and objects location in it.

Copy link
Author

Choose a reason for hiding this comment

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

Still don't see the point. In this particular case we are adding and iterating. For these operations LinkedList and ArrayList have the similar complexity, noting that ArrayList has constant size of course.

Not sure I can get what you mean saying

it is all about the heap and objects location in it
unless you provide reliable arguments.

Generally speaking if we talk about optimization I find it quite unusual that you did not mention using java arrays for example. Looks like you are arguing just for a dispute.

Copy link
Contributor

Choose a reason for hiding this comment

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

unless you clearly want to use some features of a LinkedList, ArrayList do the work in a less complex way
so basically, you should use ArrayList and as you know the size of the elements you can set in in the constructor

Copy link
Author

Choose a reason for hiding this comment

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

Okay, obviously for me it is not a big deal to replace a LinkedList with an ArrayList. But if you indeed distinguish velocity of ArrayList and LinkedList and optimization critically matters here then tell me why do you insist on using an ArrayList while java array will be even more efficient?

I guess if I know the size of an array I definitely should use simple java array (taking into account that I need only set element and get iterator).

I need to know how do you see the difference between well-optimized and over-optimized code, how do you see where optimization matters more than convenience. Where can I find any rules or conventions or you just impose your preferences? 😆



for (int i = 0; i < threadNumber; i++) {
addMockedProjectConfigDto(ptRegistry.getProjectType("my_project_type"), "my_project_name" + i);
}

IntStream.range(0, threadNumber).forEach(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why here you use forEach and just before you used a for (int i =0; loop)

Copy link

Choose a reason for hiding this comment

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

+1 foreach looks odd here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the reason that stands by using of forEach (and stream api in general) here is that it is not quite possible to access non final variables from lambda context:
addMockedProjectConfigDto(ptRegistry.getProjectType("my_project_type"), "my_project_name" + i);

This situation would happen if I used simple loop.

Do you see how we can make it less ugly?

Copy link
Contributor

Choose a reason for hiding this comment

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

my concern was only to unify the two loops into a single loop. (because we iterate on the same numbers but in a different way)
and yes to avoid hacks like int j = i; you've to use IntStream

Copy link
Author

Choose a reason for hiding this comment

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

Exactly! As we can see in this article in general cases java streams are a bit slower than ArrayList, so I would obviously use simple java loop here because it would also improve readability but the limitation is that it is not possible to refer to a non final variable.

i -> {
futures.add(executor.submit(() -> {
countDownLatch.countDown();
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, it seems that you don't have to countDown() each time, you can create a CountDownLatch with 1 as count and perform .countDown() once after forEach.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure that I got your idea, could you explain more?

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea: await 100 times, count down once.

Copy link

Choose a reason for hiding this comment

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

Please consider usage of barrier here instead of countdown latch

Copy link
Author

Choose a reason for hiding this comment

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

One major difference is that CyclicBarrier takes an (optional) Runnable task which is run once the common barrier condition is met.

It also allows you to get the number of clients waiting at the barrier and the number required to trigger the barrier. Once triggered the barrier is reset and can be used again.

For simple use cases - services starting etc... a CountdownLatch is fine. A CyclicBarrier is useful for more complex co-ordination tasks. An example of such a thing would be parallel computation - where multiple subtasks are involved in the computation - kind of like MapReduce.

http://stackoverflow.com/questions/4168772/java-concurrency-countdown-latch-vs-cyclic-barrier

There is no need to run Runnable
There are no cycles
So please could you make me understand what is the point of using CyclicBarrier instead?

countDownLatch.await();

try {
return launcher.service(DELETE,
"http://localhost:8080/api/project/my_project_name" + i,
"http://localhost:8080/api", null, null, null);
} catch (Exception e) {
throw new IllegalStateException(e);
}
}));
}
);

boolean isNotDone;
do {
isNotDone = false;
for (Future<ContainerResponse> future : futures) {
if (!future.isDone()) {
isNotDone = true;
}
}
} while (isNotDone);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can do it clearer and more gracefully by using another CountDownLatch(numThreads) and awaiting with timeout.

Copy link
Author

Choose a reason for hiding this comment

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

I have deep concerns that using

another CountDownLatch(numThreads) and awaiting with timeout

is clearer than a simple loop. Unless you can prove it?


for (Future<ContainerResponse> future : futures) {
assertEquals(future.get().getStatus(), 204, "Error: " + future.get().getEntity());
}
}

@Test
public void testCopyFile() throws Exception {
RegisteredProject myProject = pm.getProject("my_project");
Expand Down