Implement Async Create Request/Response with Oplock implementation. #354

yin19941005 · 2018-07-10T03:12:36Z

Add Oplock related messages and enum classes. Add oplock related NtStatus. Read messageId in messageConverter to determine oplock message type. Implement equals and hashCode for SMB2FileId to let user easier to compare. Implement the Async Create Request/Response and Oplock Break Notification by implementing notificationHandler with SMBEvent. Capturing the messageId of async create before send and capturing the Oplock Break Notification and Create Response on receiving in Connection and delivery them to user with ensuring the sequence is exactly the same as TCP layer. Add more create API to achieve the condition mentioned before. Public some methods to allow the user to create the diskEntry by his/her own.

Add taskQueue and test case for Async Create and oplock related implementation.

Add Oplock related messages and enum classes. Add oplock related NtStatus. Read messageId in messageConverter to determine oplock message type. Implement equals and hashCode for SMB2FileId to let user easier to compare. Implement the Async Create Request/Response and Oplock Break Notification by implementing notificationHandler with SMBEvent. Capturing the messageId of async create before send and capturing the Oplock Break Notification and Create Response on receiving in Connection and delivery them to user with ensuring the sequence is exactly the same as TCP layer. Add more create API to achieve the condition mentioned before. Public some methods to allow the user to create the diskEntry by his/her own.

…rService for notifyHandler in SmbConfig. Add taskQueue (from Vert.x Library) for DiskShare notifyHandler. The Apache-2.0 from Vert.x had been kept and the modified code had added comment. Allow user pass in executorService for notifyHandler in SmbConfig instead of always create a new executorService for each DiskShare. This can avoid having multiple thread pool when multiple DiskShare is opened.

…or oplock break classes. Add SMB2OplockBreakFactory to create oplock break notification and acknowledgement. Lease break related should also using this factory to create. Keep the implementation details in the factory and simplify the messageConverter. Add super class for all SMB2_OPLOCK_BREAK(0x12) command classes to reduce the code duplication.

…fication type. Change NotificationHandler to have single handle method for each Notification type. Adopt the changes in DiskShare. Remove the enum class for notification type as no longer used. Add abstract class for notificationHandler. This abstract class is for user not interested for all notification and can simply extends the abstract class to override for some notification.

…cense in taskQueue class to fit CI requirement.

pepijnve

It's not clear to me why the TaskQueue is needed. In this particular usage pattern it seems like submitting the Runnables directly to the Executor achieves the same thing. TaskQueue supports serialising execution of Runnables across multiple Executors, but that's not being used here as far as I can tell.

yin19941005 · 2018-07-10T08:19:40Z

Hi @hierynomus,

The taskQueue class is copied from the Vert.x. Base on my understanding to Apache License 2.0, as I keep the license and add comment for each modification, it will be fine to use it. Just special remark for this sensitive licensing issue.

yin19941005 · 2018-07-10T08:53:17Z

Hi @pepijnve,

Indeed the SingleThreadExecutor can provide the similar effect. If you are considering there are only one DiskShare instance exists, it make no difference. The difference between SingleThreadExecutor and TaskQueue is performance optimization. The goal is guarantee sequence for notification within a DiskShare, i.e. notifying DiskShare A and notifying DiskShare B at the same time is allowed. To achieve this goal, I come up with 3 approaches :

Use single thread pool for each DiskShare
Use a single thread pool for whole Smbj
Use a task queue per DiskShare and having multi-threaded thread pool for whole Smbj.

Use single thread pool for each DiskShare

Easy to implement and easy to guarantee the event sequence. But creating thread is an expensive operation operation, which should avoid. Also, having dedicated thread for each DiskShare is obviously over killed. The context switching overhead is high as well.

Use a single thread pool for whole Smbj

Easy to implement and easy to guarantee the event sequence. However, using single thread may become performance bottomneck as the asynchronous event scale up when numbers of DiskShare increase. Also, I had planned to add more and more asynchronous event in smbj (e.g. Notify, Lease), using single thread for whole Smbj may become performance bottomneck.

Use a task queue per DiskShare and having multi-threaded thread pool for whole Smbj

The user is flexible to determine how many threads they needed. Multi-threading across different DiskShare instance is allowed.

Conclusion

As a result, I think using a task queue per DiskShare and having multi-threaded thread pool for whole Smbj is the best choice.

hierynomus · 2018-07-10T08:44:22Z

src/main/java/com/hierynomus/mssmb2/SMB2OplockBreakLevel.java

+/**
+ * [MS-SMB2].pdf 2.2.23 SMB2 OPLOCK_BREAK Notification - OplockLevel
+ */
+public enum SMB2OplockBreakLevel implements EnumWithValue<SMB2OplockBreakLevel> {


Let's merge this weith the SMB2OplockLevel, they're exactly the same values. I don't see any additional benefit to separating this Enum

This is because the valid set is different between requesting oplock and breaking oplock. Only LEVEL_NONE and LEVEL_II are valid for breaking oplock. Base on my understanding, each enum class should only contain the valid values (and also null in Java). Adding valid check functions maybe is an alternative but valid check function check on run time. Separating this enum will force the checking on compile time. In my opinion, error checking on compile time is better than on run time. For me, I think simply separate this enum should make the code looks simpler than calling valid check function.

Please model it according to spec, there is no SMB2_OPLOCK_BREAK_LEVEL, only SMB2_OPLOCK_LEVEL. With different enums we need to have translation code in place to transform one into the other. Please merge them.

hierynomus · 2018-07-10T08:46:08Z

src/main/java/com/hierynomus/mssmb2/messages/SMB2OplockBreak.java

+        super();
+    }
+
+    protected SMB2OplockBreak(int structureSize, SMB2Dialect dialect, SMB2MessageCommandCode messageType, long sessionId) {


Remove the SMB2MessageCommandCode from the signature, it is always SMB2_OPLOCK_BREAK for this subtree.

Indeed, will change it.

hierynomus · 2018-07-10T08:46:45Z

src/main/java/com/hierynomus/mssmb2/messages/SMB2OplockBreakFactory.java

+
+import java.util.Arrays;
+
+public class SMB2OplockBreakFactory{


space before {

hierynomus · 2018-07-10T08:47:14Z

src/main/java/com/hierynomus/mssmb2/messages/SMB2OplockBreakFactory.java

+        buffer.skip(24);
+        byte[] messageId = buffer.readRawBytes(8);
+        buffer.rpos(0);
+        final boolean isBreakNotification = Arrays.equals(messageId, (new byte[]{(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF}));


Make the byte[] a constant.

Will add private static final long breakMessageId = -1; on SMB2OplockBreakFactory and check by final boolean isBreakNotification = messageId == breakMessageId; instead of using byte array.

hierynomus · 2018-07-10T08:47:37Z

src/main/java/com/hierynomus/mssmb2/messages/SMB2OplockBreakFactory.java

+        // final int structureSize = buffer.readUInt16();
+        // buffer.rpos(0);
+
+        if(isBreakNotification) {


spacing please :)

I guess no spacing issue on this line?

Look at the other places, a space between if and ( is missing... (Here and in other places).

hierynomus · 2018-07-10T08:50:11Z

src/main/java/com/hierynomus/protocol/commons/concurrent/FutureWrapper.java

+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class FutureWrapper<T extends U, U> implements Future<T> {


Why is this FutureWrapper here? What does it add?

Currently, FutureWrapper is used to deal with issue on Connection publishing AsyncCreateResponseNotification. In Java, Dogs is a child of Animal but List<Dogs> is not a child of List<Animal> as well as Future<Dogs> is not a child of Future<Animal>.

However, I just read the details for

// [MS-SMB2].pdf 3.2.5.1.8 Processing the Response outstandingRequests.receivedResponseFor(messageId).getPromise().deliver(packet);

and just finds that it only delivering the packet to Future and do nothings more.

I was thinking there are some process is required to handle for 3.2.5.1.8 Processing the Response so I just kept it in Future format and get the Future just as the synchronize create.

I think I should simply publish the SMB2CreateResponse on the Connection and remove this FutureWrapper class.

hierynomus · 2018-07-10T11:27:41Z

src/main/java/com/hierynomus/protocol/commons/concurrent/TaskQueue.java

@@ -0,0 +1,120 @@
+/*


Let's not copy-paste this. The license is not correct now anymore (wrong attribution).
Why aren't we using a simple LinkedBlockingQueue to queue the events?

I see your point. Let me check how to implement a "TaskQueue" with a LinkedBlockingQueue.

Hi @hierynomus,

I guess same issue happening here. The implementation had already changed on latest commit. The line we commenting is just a comment in the code so GitHub didn't mark this change request is outdated. Could you please check the implementation?

hierynomus · 2018-07-10T11:30:30Z

src/main/java/com/hierynomus/smbj/share/DiskShare.java

+    private void oplockBreakNotification(final OplockBreakNotification oplockBreakNotification) {
+        try {
+
+            final SMB2FileId fileId = oplockBreakNotification.getFileId();


Shouldn't we check here whether the OplockBreakNotification.fileId is actually handled by this DiskShare?

I don't think we need to check for fileId. The client MUST ignore it if fileId not found.

Base on document,

3.2.5.1.2 Finding the Application Request for This Response
The client MUST locate the request for which this response was sent in reply by locating the request in
Connection.OutstandingRequests using the MessageId field of the SMB2 header. If the request is
not found, the response MUST be discarded as invalid.
If the MessageId is 0xFFFFFFFFFFFFFFFF, this is not a reply to a previous request, and the client
MUST NOT attempt to locate the request, but instead process it as follows:
If the command field in the SMB2 header is SMB2 OPLOCK_BREAK, it MUST be processed as specified
in 3.2.5.19. Otherwise, the response MUST be discarded as invalid.

3.2.5.19.1 Receiving an Oplock Break Notification
The client MUST locate the open in the Session.OpenTable using the FileId in the Oplock Break
Notification following the SMB2 header. If the open is not found, the oplock break indication MUST be
discarded, and no further processing is required.

And currently, it seems the DiskShare didn't record for the openedHandle. I don't think we should add it just because an unnecessary checking.

But indeed reminds me something. The fileId is only unique within a session. We should check the oplockBreakNotification.getHeader().getSessionId() match DiskShare.session.getSessionId() or not before notifying the user.

At this moment multiple DiskShares will be receiving the message from the bus. You need to check which one needs to process the message, so you do need to check both the sessionId as well as the fileId.

Just to be sure, is it sufficient to confirm the DiskShare is same by checking the oplockBreakNotification.getHeader().getTreeId() match DiskShare.treeConnect.getTreeId()?

Update
Checking for `treeId` and `sessionId` should works for async create request/response. However, It seems Windows server would simply set both `treeId` and `sessionId` to `0` with messageId `-1` for `OplockBreakNotification`.

Ok, that makes that we need to use a contextual bus per connection. Because it could very well be that the same fileId exists on multiple different connections (to different hosts).

Hi @hierynomus,

The latest commit already using a contextual bus per connection. The fix is on commit a48ad30. I guess this line we are commenting didn't change at all. So, the GitHub didn't mark this change request is outdated automatically. Could you please check the implementation?

hierynomus · 2018-07-10T11:31:47Z

src/main/java/com/hierynomus/smbj/share/DiskShare.java

+                }, notifyExecutor);
+            }else {
+                logger.warn("FileId {}, NotificationHandler not exist to handle Oplock Break.", fileId);
+                throw new IllegalStateException("NotificationHandler not exist to handle Oplock Break.");


Why is this an exception, and the createRequestNotification does not throw an exception?

Because the createRequestNotification and createResponseNotification is always called even you are using synchronize create (the existing create). The is a valid use case for a user to using the synchronize create and not set the NotificationHandler (as this is intended to handle async event only).

However, it is not a valid use case for using any level of oplock higher than None on any kind of create command. Because the OplockBreakNotification is always "Async" (or I should say Server initiated?).

codecov · 2018-07-10T11:34:04Z

Codecov Report

Merging #354 into master will decrease coverage by 0.53%.
The diff coverage is 32.94%.

@@             Coverage Diff              @@
##             master     #354      +/-   ##
============================================
- Coverage     45.79%   45.25%   -0.54%     
- Complexity      870      892      +22     
============================================
  Files           217      231      +14     
  Lines          6457     6697     +240     
  Branches        471      486      +15     
============================================
+ Hits           2957     3031      +74     
- Misses         3257     3417     +160     
- Partials        243      249       +6

Impacted Files	Coverage Δ	Complexity Δ
...n/java/com/hierynomus/smbj/share/PrinterShare.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...main/java/com/hierynomus/smbj/share/PipeShare.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...mssmb2/messages/SMB2OplockBreakAcknowledgment.java	`0% <0%> (ø)`	`0 <0> (?)`
...mbj/event/handler/AbstractNotificationHandler.java	`0% <0%> (ø)`	`0 <0> (?)`
...hierynomus/smbj/event/OplockBreakNotification.java	`0% <0%> (ø)`	`0 <0> (?)`
...essages/SMB2OplockBreakAcknowledgmentResponse.java	`0% <0%> (ø)`	`0 <0> (?)`
...va/com/hierynomus/mssmb2/SMB2OplockBreakLevel.java	`0% <0%> (ø)`	`0 <0> (?)`
...om/hierynomus/mssmb2/messages/SMB2OplockBreak.java	`0% <0%> (ø)`	`0 <0> (?)`
...mssmb2/messages/SMB2OplockBreakServerResponse.java	`0% <0%> (ø)`	`0 <0> (?)`
...main/java/com/hierynomus/smbj/share/DiskEntry.java	`11.32% <0%> (-0.68%)`	`1 <0> (ø)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e23e124...cf27e09. Read the comment docs.

hierynomus · 2018-07-11T09:22:12Z

src/main/java/com/hierynomus/mssmb2/SMB2OplockBreakLevel.java

+/**
+ * [MS-SMB2].pdf 2.2.23 SMB2 OPLOCK_BREAK Notification - OplockLevel
+ */
+public enum SMB2OplockBreakLevel implements EnumWithValue<SMB2OplockBreakLevel> {


Please model it according to spec, there is no SMB2_OPLOCK_BREAK_LEVEL, only SMB2_OPLOCK_LEVEL. With different enums we need to have translation code in place to transform one into the other. Please merge them.

…on. Return SMB2CreateResponse instead of Future in asyncNotification. Minor Fixes. Make breakMessageId constant in SMB2OplockBreakFactory. Directly return SMB2CreateResponse instead of Future in AsyncCreateResponseNotification. Remove FutureWrapper class. Fix equals not implemented correctly in SMB2FileId. Fix didn't check should handle or not when the DiskShare received a async notification. Didn't simply check the treeId in asyncCreateResponseNotification is because the Server may response an "Asynchronous Responses". At that case, treeId is not exist in header.

…n19941005/smbj into implement-async-create-oplock

yin19941005 · 2018-07-13T10:36:35Z

Hi @hierynomus,

The commit I just pushed should fixed some issue. The remaining outstanding issue :

TODO List:

Merge SMB2_OPLOCK_LEVEL and SMB2_OPLOCK_BREAK_LEVEL
Remove the SMB2MessageCommandCode from the signature, it is always SMB2_OPLOCK_BREAK for this subtree.
Code format fix
Implement TaskQueue with a LinkedBlockingQueue
Add a Connection internal EventBus for each Connection. Use the internal EventBus instead of the global EventBus for every Notification.
Make AbstractAsyncNotification is used by Async Request only. (To recording messageId and used for should handle when receive in Async Response)
Implement a internal centralize messageId callback to record all implemented notificationHandler MessageId?

Please let me know if the list missed some issue you had mentioned before.

For issue 6 and 7, I guess while I complete issue 7 and AbstractAsyncNotification will just disappear and issue 6 no longer exist. But I keep it on list for reference.

…ance smb2oplock classes. Enhance notification classes. Re-implement taskQueue. Fix publishing connection internal event through client global EventBus. Separating the client global EventBus and connection internal EventBus and only passing the connection internal EventBus to all the classes under connection (e.g. session). Enhance SMB2OplockBreak class constructor to reduce code. Enhance notification classes. The Async request notification would use sessionId and treeId to check the DiskShare is required to handle while the Async response notification would use messageId to check the DiskShare is required to handle. Add internal notification for notifying all the messageId of the supported AsyncOperation on the DiskShare. Re-implement the taskQueue class. Update test case to adopt changes.

…cutorService in config.

Base on SMB2 document, there are no SMB2_OPLOCK_BREAK_LEVEL. Merge to SMB2_OPLOCK_LEVEL and remove SMB2_OPLOCK_BREAK_LEVEL.

yin19941005 · 2018-08-10T09:45:24Z

Hi @hierynomus,

I guess I had already change the code to fit all your change request.

For TaskQueue, I had changed it as interface (default using single thread executor) and allow user to pass their own TaskQueue in to use in config. I tried to implement it but I didn't sure I had implemented it correctly so I change it as interface for more flexibility. Is this okay for you?

hierynomus · 2018-08-17T08:36:06Z

Can you take a look at the codacy messages? There are a few degradations in quality.

yin19941005 · 2018-08-17T09:09:35Z

No problem. Will try to fix the codacy stuffs.

yin19941005 · 2018-08-20T09:40:04Z

Hi @hierynomus,

The codacy issue had been fixed. However I still got error:

security/snyk - build.gradle (hierynomus (GitHub marketplace)) — Failed to detect issues

I guess this is caused by this branch is out of date?

yin19941005 added 6 commits July 4, 2018 18:26

Add test case for async create and oplock lock related.

86b602b

Reverse the order of SMBJ Apache license and Eclipse Vert.x Apache li…

9df5b0f

…cense in taskQueue class to fit CI requirement.

pepijnve reviewed Jul 10, 2018

View reviewed changes

hierynomus requested changes Jul 10, 2018

View reviewed changes

Merge branch 'master' into implement-async-create-oplock

0dc92ae

hierynomus requested changes Jul 11, 2018

View reviewed changes

yin19941005 added 2 commits July 13, 2018 18:28

Merge branch 'implement-async-create-oplock' of https://github.com/yi…

b4aee52

…n19941005/smbj into implement-async-create-oplock

yin19941005 added 4 commits July 17, 2018 16:13

Change TaskQueue to interface and allow set by config. Remove set exe…

a3fed23

…cutorService in config.

Merge SMB2_OPLOCK_BREAK_LEVEL to SMB2_OPLOCK_LEVEL.

eedacd5

Base on SMB2 document, there are no SMB2_OPLOCK_BREAK_LEVEL. Merge to SMB2_OPLOCK_LEVEL and remove SMB2_OPLOCK_BREAK_LEVEL.

Remove unused import

e8517b6

Codacy Cleanup

cf27e09


		import java.util.Arrays;

		public class SMB2OplockBreakFactory{

Implement Async Create Request/Response with Oplock implementation. #354

Are you sure you want to change the base?

Implement Async Create Request/Response with Oplock implementation. #354

Conversation

yin19941005 commented Jul 10, 2018

pepijnve left a comment

Choose a reason for hiding this comment

yin19941005 commented Jul 10, 2018

yin19941005 commented Jul 10, 2018

Use single thread pool for each DiskShare

Use a single thread pool for whole Smbj

Use a task queue per DiskShare and having multi-threaded thread pool for whole Smbj

Conclusion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yin19941005 Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Update

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

yin19941005 commented Jul 13, 2018

yin19941005 commented Aug 10, 2018

hierynomus commented Aug 17, 2018

yin19941005 commented Aug 17, 2018

yin19941005 commented Aug 20, 2018

yin19941005 Jul 12, 2018 •

edited

Loading

codecov bot commented Jul 10, 2018 •

edited

Loading