add function body to NotifyChange and Cancel

[ksa-triovega]

Hi,

I added function body to your functions:

• NotifyChange(...)
• Cancel(...)

Is it okay to use: ThreadPool.QueueUserWorkItem(()=>...) here? Or would you rather have sync call (without extra thread)?

The lines:

private readonly Dictionary<object, bool> _activeNotifies;
private readonly object _lock = new object();

are only to cancel the recursive call.

Now the client can use it like the following code:

object directoryHandle;
FileStatus fileStatus;
NTStatus createStatus = smb2FileStore.CreateFile(
    out directoryHandle,
    out fileStatus,
    "",
    AccessMask.GENERIC_READ,
    SMBLibrary.FileAttributes.Directory,
    ShareAccess.Read | ShareAccess.Write,
    CreateDisposition.FILE_OPEN,
    CreateOptions.FILE_DIRECTORY_FILE,
    null);

NTStatus notifyStatus = smb2FileStore.NotifyChange(
    out m_ioRequest, 
    directoryHandle, 
    NotifyChangeFilter.FileName | NotifyChangeFilter.DirName | NotifyChangeFilter.Size, 
    true, 
    int.Parse(bufferSizeTextBox.Text), 
    OnNotifyChangeCompleted,  // Your callback
    null); // Custom context, here null

PS: I will add unit tests later, if the concept idea is okay.

[ksa-triovega]

Hey,

I would like to move this forward whenever you get a chance. Let me know if there is anything I can adjust to make the review easier.

[TalAloni]

I appreciate you providing this new functionality and contributing this code.
This is an open source project that I maintain on my spare time, in the very limited time I have I focus on fixing bugs and resolving issues around existing functionality.

New functionality has to be vetted extensively to ensure that it's usable by most users and that I can support it,
I want to take a close look at providing ChangeNotify support for the client - and evaluate whether your code is the best code - I have more things I want to do than time to do them - I will try to get to it when time permits.

[TalAloni]

Hi, apologies for the delay in getting to this

1. IIUC this logic may not work when the notification is received after the command timeout (5 seconds by default)
2. Did you consider a direct invocation of onNotifyChangeCompleted instead of using the thread pool?
3. I think it's up to the user to re-subscribe for additional changes

[ksa-triovega]

Hi, thank you for your feedback.

1. this is correct you can set timeout using the connect function as parameter, but I see your point and added response type timeout.
2. Yes, I was/am just not sure if it is better if library handles the resubscription or the user of library in regard to speed and potentially missing an event if there are a lot of changes in short time.
3. And you are correct, not everybody want a continues flow of events.

I removed the thread from library then and that means the user of the library has to wrap it into a thread and resubscribe if he wishes it to.

Is this more appropriate?

[TalAloni]

Shouldn't cancellation be sent to the server?

[TalAloni]

Again apologies for the delay,
Ideally the client should "behaves" as similar as a local filesystem would behave, this means that I'm expecting that in most cases NotifyChange will return NTStatus.STATUS_PENDING and in those cases I'm able to cancel the request and I'm expecting that the OnNotifyChangeCompleted the caller provides will be invoked when applicable.
(This does not mean you have to spin up new threads or queue additional threadpool work)