Pass vector element as a function parameter

[chris db]

I have a global vector of structs that is dynamically resized, and the way I use it is by push_back elements in the vector and pass a reference to that element to a function that is going to use that pointer as a context variable for each callback.

typedef struct{
	...
} DOWNLOAD_CONTEXT;

// global variable
std::vector<DOWNLOAD_CONTEXT> contexts;

// asynchronous function called whenever requested
void Download(TCHAR *url)
{
    DOWNLOAD_CONTEXT context;
    context.prop = 
    ...
    contexts.push_back(context);
    InternetSetStatusCallback(hInternet, DownloadProgress);
    InternetOpenUrl(hInternet, url, _T(""), 0, INTERNET_FLAG_RELOAD,
         DWORD_PTR(&*(contexts.end() - 1))); // last parameter is the session specific context
}

// callback function called multiple times by the system, passing the associated context
void CALLBACK DownloadProgress(
	_In_  HINTERNET hInternet,
	_In_  DWORD_PTR dwContext,
	_In_  DWORD dwInternetStatus,
	_In_  LPVOID lpvStatusInformation,
	_In_  DWORD dwStatusInformationLength
	)
{
    DOWNLOAD_CONTEXT *context = (DOWNLOAD_CONTEXT*)dwContext;
    switch (dwInternetStatus)
    {
         ...
    }
}

This works, but if I rapidly fire Download() twice, sometimes the callback function gets a corrupted DOWNLOAD_CONTEXT pointer.

So how can I pass a vector element to a function, considering that the vector is dynamically resized with push_back and erase?
NOTE: It doesn't operate like a stack, elements in the middle can be removed.

[cire]

This is a situation where using a std::list rather than a vector or deque might be indicated.

Your subject is misleading. You are not passing a vector iterator.

[chris db]

Ok, std::list works much better, the context seems to be correct every time.

Just to be clear, is it ok the way I pass the pointer to the list element (I know it's not the actual iterator) ? Does it go out of scope ?

std::list<DOWNLOAD_CONTEXT> contexts;
void Download(TCHAR *url)
{
    DOWNLOAD_CONTEXT context;
    list<DOWNLOAD_CONTEXT>::iterator it;
    it = contexts.insert(contexts.begin(), context);
    InternetOpenUrl(hInternet, url, _T(""), 0, INTERNET_FLAG_RELOAD,
        DWORD_PTR(&*it)); // will it be ok when the callback will accesses this pointer ?
}

[dhayden]

This works, but if I rapidly fire Download() twice, sometimes the callback function gets a corrupted DOWNLOAD_CONTEXT pointer.

Does that mean this code is multithreaded? If so then you have to do something to guarantee that the context doesn't move as a result of another call (like the second call to Download). For example:

- You call Download and pass a reference to the first item in contexts. The first download starts.
- You call Download again. This time contexts is full so when you add an item, the vector gets moved when expanded.
- The next time that the first Download calls DownloadProgress(), the dwContext parameter points to unallocated memory.

Why put the context in the global container at all? Why put the context in the collection at all? Either pass the local context variable into InternetOpenURL (if that function runs to completion and then returns), or pass in a context instance to use. Modifying globals is usually a bad idea, and in multi-threaded programs it's a REALLY bad idea.

[ne555]

> This is a situation where using a std::list rather than a vector or deque might be indicated
or you can simply use the index of the element.

[chris db]

No, the code is not multithreaded, is single threaded.
Download() is triggered by a GUI element, and since InternetOpenUrl() is executed asynchronous (it returns immediately), Download() returns before the actual download is finished, so you can have multiple downloads running concurrently by the virtue of asynchronous WinINet functions, that report back to the designated callback function.

If I don't store the context in a global container, wouldn't it go out of scope when Download() returns? And then when the callback would try to access it, it would get access violation.

Anyway, using the std::list the way I showed above, seems to work so far. I tested multiple downloads and everything was fine. I'm not 100% sure though.

[dhayden]

Thanks for the explanation. The program is multi-threaded, it's just that you aren't creating the thread. InternetOpenUrl() is doing that. The problem with the vector is exactly as I described: When you add a new download, the vector gets moved, but a previous call to INternetOpenUrl is still using one of the old values. The reason a list works is that when you add something to a list, it stays put.