Skip to content

Commit

Permalink
GH-3370: Remove synchronized from RemoteFileUtils (#3380)
Browse files Browse the repository at this point in the history 
* GH-3370: Remove synchronized from RemoteFileUtils

Fixes #3370

The `synchronized` on the `RemoteFileUtils.makeDirectories()` makes an application too
 slow, especially when we deal with different paths in different sessions

* Remove the `synchronized` from that method and rework `SftpSession.mkdir()`
to return `false` when "A file cannot be created if it already exists" exception
is thrown from the server.
Essentially make an `exists()` call to be sure that an exception is really related
to "file-already-exists" answer from the server

**Cherry-pick to 5.3.x, 5.2.x & 4.3.x**

* * Re-throw an exception in the `SftpSession.mkdir()`
when error code is not `4` or remote dir does not exist

* * Check `session.mkdir()` result in the
`RemoteFileUtils` to throw an `IOException` when `false`

* * Fix mock test to return `true` for `mkdir` instead of `null`
# Conflicts:
#	spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileUtils.java
#	spring-integration-file/src/main/java/org/springframework/integration/file/remote/gateway/AbstractRemoteFileOutboundGateway.java
#	spring-integration-file/src/test/java/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java
#	spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java
  • Loading branch information
@artembilan
artembilan committed Sep 10, 2020
1 parent 059d474 commit 9019676
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 45 deletions.
18 changes: 12 additions & 6 deletions ...ation-file/src/main/java/org/springframework/integration/file/remote/RemoteFileUtils.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -27,6 +27,8 @@
/**
* Utility methods for supporting remote file operations.
* @author Gary Russell
* @author Artem Bilan
*
* @since 3.0
*
*/
Expand All @@ -45,13 +47,11 @@ private RemoteFileUtils() {
* @param logger The logger.
* @throws IOException Any IOException.
*/
public static synchronized <F> void makeDirectories(String path, Session<F> session, String remoteFileSeparator,
public static <F> void makeDirectories(String path, Session<F> session, String remoteFileSeparator,
Log logger) throws IOException {

if (!session.exists(path)) {

int nextSeparatorIndex = path.lastIndexOf(remoteFileSeparator);

if (nextSeparatorIndex > -1) {
List<String> pathsToCreate = new LinkedList<String>();
while (nextSeparatorIndex > -1) {
Expand All @@ -70,13 +70,19 @@ public static synchronized <F> void makeDirectories(String path, Session<F> sess
if (logger.isDebugEnabled()) {
logger.debug("Creating '" + pathToCreate + "'");
}
session.mkdir(pathToCreate);
tryCreateRemoteDirectory(session, pathToCreate);
}
}
else {
session.mkdir(path);
tryCreateRemoteDirectory(session, path);
}
}
}

private static void tryCreateRemoteDirectory(Session<?> session, String path) throws IOException {
if (!session.mkdir(path)) {
throw new IOException("Could not create a remote directory: " + path);
}
}

}
69 changes: 34 additions & 35 deletions ...rg/springframework/integration/file/remote/gateway/AbstractRemoteFileOutboundGateway.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2017 the original author or authors.
* Copyright 2002-2020 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -510,14 +510,13 @@ protected void doInit() {
if (Command.MGET.equals(this.command)) {
Assert.isTrue(!(this.options.contains(Option.SUBDIRS)),
"Cannot use " + Option.SUBDIRS.toString() + " when using 'mget' use "
+ Option.RECURSIVE.toString() + " to obtain files in subdirectories");
+ Option.RECURSIVE.toString() + " to obtain files in subdirectories");
}

if (getBeanFactory() != null) {
if (this.fileNameProcessor != null) {
this.fileNameProcessor.setBeanFactory(getBeanFactory());
}

this.renameProcessor.setBeanFactory(getBeanFactory());
this.remoteFileTemplate.setBeanFactory(getBeanFactory());
}
Expand All @@ -527,20 +526,20 @@ protected void doInit() {
protected Object handleRequestMessage(final Message<?> requestMessage) {
if (this.command != null) {
switch (this.command) {
case LS:
return doLs(requestMessage);
case GET:
return doGet(requestMessage);
case MGET:
return doMget(requestMessage);
case RM:
return doRm(requestMessage);
case MV:
return doMv(requestMessage);
case PUT:
return doPut(requestMessage);
case MPUT:
return doMput(requestMessage);
case LS:
return doLs(requestMessage);
case GET:
return doGet(requestMessage);
case MGET:
return doMget(requestMessage);
case RM:
return doRm(requestMessage);
case MV:
return doMv(requestMessage);
case PUT:
return doPut(requestMessage);
case MPUT:
return doMput(requestMessage);
}
}
return this.remoteFileTemplate.execute(new SessionCallback<F, Object>() {
Expand Down Expand Up @@ -570,8 +569,8 @@ public List<?> doInSession(Session<F> session) throws IOException {
}
});
return this.getMessageBuilderFactory().withPayload(payload)
.setHeader(FileHeaders.REMOTE_DIRECTORY, dir)
.build();
.setHeader(FileHeaders.REMOTE_DIRECTORY, dir)
.build();
}

private Object doGet(final Message<?> requestMessage) {
Expand Down Expand Up @@ -621,9 +620,9 @@ public List<File> doInSession(Session<F> session) throws IOException {
}
});
return this.getMessageBuilderFactory().withPayload(payload)
.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
.build();
.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
.build();
}

private Object doRm(Message<?> requestMessage) {
Expand All @@ -634,24 +633,24 @@ private Object doRm(Message<?> requestMessage) {
boolean payload = this.remoteFileTemplate.remove(remoteFilePath);

return this.getMessageBuilderFactory().withPayload(payload)
.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
.build();
.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
.build();
}

private Object doMv(Message<?> requestMessage) {
String remoteFilePath = this.fileNameProcessor.processMessage(requestMessage);
String remoteFilePath = this.fileNameProcessor.processMessage(requestMessage);
String remoteFilename = getRemoteFilename(remoteFilePath);
String remoteDir = getRemoteDirectory(remoteFilePath, remoteFilename);
String remoteFileNewPath = this.renameProcessor.processMessage(requestMessage);
Assert.hasLength(remoteFileNewPath, "New filename cannot be empty");

this.remoteFileTemplate.rename(remoteFilePath, remoteFileNewPath);
return this.getMessageBuilderFactory().withPayload(Boolean.TRUE)
.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
.setHeader(FileHeaders.RENAME_TO, remoteFileNewPath)
.build();
.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
.setHeader(FileHeaders.RENAME_TO, remoteFileNewPath)
.build();
}

private String doPut(Message<?> requestMessage) {
Expand Down Expand Up @@ -722,7 +721,7 @@ private List<String> putLocalDirectory(Message<?> requestMessage, File file, Str
else if (this.options.contains(Option.RECURSIVE)) {
String newSubDirectory = (StringUtils.hasText(subDirectory) ?
subDirectory + this.remoteFileTemplate.getRemoteFileSeparator() : "")
+ filteredFile.getName();
+ filteredFile.getName();
replies.addAll(this.putLocalDirectory(requestMessage, filteredFile, newSubDirectory));
}
}
Expand Down Expand Up @@ -795,7 +794,7 @@ private List<F> listFilesInRemoteDir(Session<F> session, String directory, Strin
}
}
if (recursion && this.isDirectory(file) && !(".".equals(fileName)) && !("..".equals(fileName))) {
lsFiles.addAll(listFilesInRemoteDir(session, directory, subDirectory + fileName
lsFiles.addAll(listFilesInRemoteDir(session, directory, subDirectory + fileName
+ this.remoteFileTemplate.getRemoteFileSeparator()));
}
}
Expand Down Expand Up @@ -858,7 +857,7 @@ protected void purgeDots(List<F> lsFiles) {
* @throws IOException Any IOException.
*/
protected File get(Message<?> message, Session<F> session, String remoteDir, String remoteFilePath,
String remoteFilename, boolean lsFirst) throws IOException {
String remoteFilename, boolean lsFirst) throws IOException {
F[] files = null;
if (lsFirst) {
files = session.list(remoteFilePath);
Expand Down Expand Up @@ -931,7 +930,7 @@ else if (FileExistsMode.IGNORE != fileExistsMode) {
}

protected List<File> mGet(Message<?> message, Session<F> session, String remoteDirectory,
String remoteFilename) throws IOException {
String remoteFilename) throws IOException {
if (this.options.contains(Option.RECURSIVE)) {
if (logger.isWarnEnabled() && !("*".equals(remoteFilename))) {
logger.warn("File name pattern must be '*' when using recursion");
Expand Down Expand Up @@ -1018,7 +1017,7 @@ private List<File> mGetWithRecursion(Message<?> message, Session<F> session, Str
String fileName = this.getRemoteFilename(fullFileName);
String actualRemoteDirectory = this.getRemoteDirectory(fullFileName, fileName);
File file = get(message, session, actualRemoteDirectory,
fullFileName, fileName, false);
fullFileName, fileName, false);
if (this.options.contains(Option.PRESERVE_TIMESTAMP)) {
file.setLastModified(getModified(lsEntry.getFileInfo()));
}
Expand Down
2 changes: 1 addition & 1 deletion ...a/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java
View file
Expand Up @@ -323,7 +323,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
madeDirs.add((String) invocation.getArguments()[0]);
return null;
return true;
}
}).when(session).mkdir(anyString());
when(sessionFactory.getSession()).thenReturn(session);
Expand Down
6 changes: 4 additions & 2 deletions ...egration-sftp/src/main/java/org/springframework/integration/sftp/session/SftpSession.java
View file
Expand Up @@ -229,8 +229,10 @@ public boolean mkdir(String remoteDirectory) throws IOException {
try {
this.channel.mkdir(remoteDirectory);
}
catch (SftpException e) {
throw new NestedIOException("failed to create remote directory '" + remoteDirectory + "'.", e);
catch (SftpException ex) {
if (ex.id != ChannelSftp.SSH_FX_FAILURE || !exists(remoteDirectory)) {
throw new NestedIOException("failed to create remote directory '" + remoteDirectory + "'.", ex);
}
}
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion ...n-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java
View file
Expand Up @@ -216,7 +216,7 @@ public void testMkDir() throws Exception {
@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
madeDirs.add((String) invocation.getArguments()[0]);
return null;
return true;
}
}).when(session).mkdir(anyString());
handler.handleMessage(new GenericMessage<String>("qux"));
Expand Down

0 comments on commit 9019676

Please sign in to comment.